Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Leftovers from demonstrating cds.Map #443

Merged
merged 3 commits into from
Feb 28, 2025
Merged

Leftovers from demonstrating cds.Map #443

merged 3 commits into from
Feb 28, 2025

Conversation

mofterdinger
Copy link
Member

@mofterdinger mofterdinger commented Feb 28, 2025

  • Remove field details from Books entity
  • Avoid duplicate items in select list

@mofterdinger mofterdinger changed the title provide default value for pages if it's missing Leftovers from demonstrating cds.Map Feb 28, 2025
@mofterdinger
Copy link
Member Author

This PR provides just some fixes for review feedback, that was mentioned after the feature was already merged. If we decide to provide another showcase for the cds.Map, I suggest to do it in a different PR.

@agoerler
Copy link
Contributor

It just crossed by mind that @OlenaTi is also introducing "pages" in this PR: https://github.com/SAP-samples/cloud-cap-samples-java/pull/428/files#diff-d92b8eb29957a03ad9d23b01feff79da293e30183d0b8eac5d893d95632e30dfR62

Maybe we should store something else in the map then.

@etimr
Copy link
Contributor

etimr commented Feb 28, 2025

It just crossed by mind that @OlenaTi is also introducing "pages" in this PR: https://github.com/SAP-samples/cloud-cap-samples-java/pull/428/files#diff-d92b8eb29957a03ad9d23b01feff79da293e30183d0b8eac5d893d95632e30dfR62

Maybe we should store something else in the map then.

I'm not sure that there is a conflict between these two things. I'm introducing a page number in the table of context, here, if I get it correctly, we are talking about the page count in the whole book.
Screenshot 2025-02-28 at 14 48 51

@mofterdinger
Copy link
Member Author

Ok, seems to be a conflict, will remove the details including pages again.

Copy link
Contributor

@beckermarc beckermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but it doesn't keep much of Map left (which is fine with me).

@mofterdinger mofterdinger merged commit 1cc55c4 into main Feb 28, 2025
2 checks passed
@mofterdinger mofterdinger deleted the leftovers_map branch February 28, 2025 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants