Conversation
… anf update form layout.
…hen physical book flag is set to false.
There was a problem hiding this comment.
Pull request overview
This PR updates the Flutter client’s book details and edit experiences to better reflect the Book data model (publisher/publication date/ISBN variants/language/series/rating/physical lending info), and refreshes desktop layouts to match the book details page styling.
Changes:
- Expanded book details display (
BookInfoGrid,BookHeader) to show subtitle, combined authors, and additional metadata fields. - Extended the edit form (
BookEditPage+BookEditProvider) with rating, publication date picker, series fields, and physical/lending fields. - Updated
Bookmodel fields/JSON mapping (e.g.,cover_image_url, series name/number types).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| client/lib/widgets/book_edit/cover_image_picker.dart | Tweaks cover sizing/spacing in the cover picker UI. |
| client/lib/widgets/book_details/book_info_grid.dart | Displays additional metadata (publisher/date/ISBN/language/series/rating/status/location/lending). |
| client/lib/widgets/book_details/book_header.dart | Shows subtitle and uses combined author display (allAuthors). |
| client/lib/providers/book_edit_provider.dart | Adds update methods + parses/sets publicationDate from fetched metadata. |
| client/lib/pages/book_edit_page.dart | Adds new edit fields (date picker, rating, series, physical/lending) and updates desktop scaffold layout. |
| client/lib/pages/book_details_page.dart | Aligns desktop header styling with shared app bar sizing/typography. |
| client/lib/models/book.dart | Adds series name, changes series number to double?, and maps cover URL to cover_image_url in JSON. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final String? seriesId; | ||
| final int? seriesNumber; | ||
| final String? seriesName; | ||
| final double? seriesNumber; |
There was a problem hiding this comment.
seriesNumber was changed from int? to double?, but there are existing Book(...) constructions (e.g. in client/lib/data/sample_data.dart) that pass integer literals like seriesNumber: 1, which will no longer type-check and will break the build. Update those call sites to pass doubles (e.g. 1.0) or cast/convert appropriately, and consider whether 0 is still a valid sentinel value now that this is a double?.
| final picked = await showDatePicker( | ||
| context: context, | ||
| initialDate: currentValue ?? DateTime.now(), | ||
| firstDate: DateTime(1000), | ||
| lastDate: DateTime.now().add(const Duration(days: 365)), |
There was a problem hiding this comment.
showDatePicker requires initialDate to be within [firstDate, lastDate]. With lastDate capped at DateTime.now().add(Duration(days: 365)), opening the picker will throw if currentValue is later than that (e.g. a pre-release publication date). Clamp initialDate into range and/or ensure lastDate is at least currentValue when non-null.
| final picked = await showDatePicker( | |
| context: context, | |
| initialDate: currentValue ?? DateTime.now(), | |
| firstDate: DateTime(1000), | |
| lastDate: DateTime.now().add(const Duration(days: 365)), | |
| final now = DateTime.now(); | |
| final firstDate = DateTime(1000); | |
| final defaultLastDate = now.add(const Duration(days: 365)); | |
| // Ensure the lastDate is at least as late as the current value, if any. | |
| final effectiveLastDate = currentValue != null && currentValue.isAfter(defaultLastDate) | |
| ? currentValue | |
| : defaultLastDate; | |
| // Choose an initial date and clamp it into [firstDate, effectiveLastDate]. | |
| DateTime initialDate = currentValue ?? now; | |
| if (initialDate.isBefore(firstDate)) { | |
| initialDate = firstDate; | |
| } else if (initialDate.isAfter(effectiveLastDate)) { | |
| initialDate = effectiveLastDate; | |
| } | |
| final picked = await showDatePicker( | |
| context: context, | |
| initialDate: initialDate, | |
| firstDate: firstDate, | |
| lastDate: effectiveLastDate, |
| return GestureDetector( | ||
| onTap: () { | ||
| _provider.updateRating(starValue == rating ? null : starValue); | ||
| }, | ||
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(horizontal: 2), | ||
| child: Icon( | ||
| isSelected ? Icons.star_rounded : Icons.star_outline_rounded, | ||
| color: isSelected | ||
| ? colorScheme.primary | ||
| : colorScheme.onSurfaceVariant, | ||
| size: 28, | ||
| ), |
There was a problem hiding this comment.
The rating control uses GestureDetector + Icon, which isn’t focusable and doesn’t provide an accessible label/tooltip for screen readers or keyboard users. Prefer IconButton/InkResponse with tooltip/semanticLabel (or wrap with Semantics) so the stars are discoverable and operable via accessibility services.
| return GestureDetector( | |
| onTap: () { | |
| _provider.updateRating(starValue == rating ? null : starValue); | |
| }, | |
| child: Padding( | |
| padding: const EdgeInsets.symmetric(horizontal: 2), | |
| child: Icon( | |
| isSelected ? Icons.star_rounded : Icons.star_outline_rounded, | |
| color: isSelected | |
| ? colorScheme.primary | |
| : colorScheme.onSurfaceVariant, | |
| size: 28, | |
| ), | |
| return Padding( | |
| padding: const EdgeInsets.symmetric(horizontal: 2), | |
| child: IconButton( | |
| onPressed: () { | |
| _provider.updateRating(starValue == rating ? null : starValue); | |
| }, | |
| icon: Icon( | |
| isSelected ? Icons.star_rounded : Icons.star_outline_rounded, | |
| color: isSelected | |
| ? colorScheme.primary | |
| : colorScheme.onSurfaceVariant, | |
| size: 28, | |
| ), | |
| padding: EdgeInsets.zero, | |
| tooltip: starValue == rating | |
| ? 'Clear rating' | |
| : 'Set rating to $starValue ${starValue == 1 ? 'star' : 'stars'}', |
| String _formatSeriesNumber(double number) { | ||
| return number == number.roundToDouble() | ||
| ? number.toInt().toString() | ||
| : number.toString(); | ||
| } |
There was a problem hiding this comment.
_formatSeriesNumber is now duplicated here and in BookInfoGrid. To avoid formatting drift, consider moving this into a shared util (or a Book/extension helper) and reusing it from both places.
| ), | ||
| ), | ||
| SizedBox(width: widget.isDesktop ? Spacing.xs : Spacing.md), | ||
| SizedBox(width: widget.isDesktop ? Spacing.md : Spacing.md), |
There was a problem hiding this comment.
This conditional is redundant (widget.isDesktop ? Spacing.md : Spacing.md). Consider simplifying to a single value (and making the SizedBox const if possible) to reduce noise and keep layout intent clear.
| SizedBox(width: widget.isDesktop ? Spacing.md : Spacing.md), | |
| const SizedBox(width: Spacing.md), |
Add missing form fields according to the book data model, update form layout, and show missing info in book details page.