Skip to content

BL-14362 fix spacing around cards without much text#68

Merged
StephenMcConnel merged 1 commit intomainfrom
BL-14362_spacing_fix
Feb 24, 2025
Merged

BL-14362 fix spacing around cards without much text#68
StephenMcConnel merged 1 commit intomainfrom
BL-14362_spacing_fix

Conversation

@nabalone
Copy link
Collaborator

@nabalone nabalone commented Feb 22, 2025

This change is Reviewable

Copy link
Member

@StephenMcConnel StephenMcConnel left a comment

Choose a reason for hiding this comment

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

Looks reasonable, but I have a couple of questions.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nabalone)


components/language-chooser/react/language-chooser-react-mui/src/OptionCard.tsx line 51 at r1 (raw file):

          // fill up the entire card action area
          flex-grow: 1;
          width: 100%;

What happens if there are fewer cards than needed to fill up the card action area?
Could cards that display only the language name omit some content that adds to their height? (just asking)

Copy link
Collaborator Author

@nabalone nabalone left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @StephenMcConnel)


components/language-chooser/react/language-chooser-react-mui/src/OptionCard.tsx line 51 at r1 (raw file):

Previously, StephenMcConnel (Steve McConnel) wrote…

What happens if there are fewer cards than needed to fill up the card action area?
Could cards that display only the language name omit some content that adds to their height? (just asking)

I'm not sure if I understand the question correctly...I'll try explaining my thinking from the beginning and see if that makes sense? Every card has its own CardActionArea, which I am using only for the purpose of detecting clicks i.e. turning the card into a button. So I think we definitely want the Card and the CardActionArea to be the same size, so that we respond to clicks that are on the card and only on the card. For language cards, the parent sets a minimum height on the OptionCard, which sets the minimum height of the CardActionArea. Previously yes, some cards would omit some content and therefore be shorter, shorter than this minimum height. So in this PR I want to make the Card grow in such cases so that it is never shorter than its CardActionArea. I put some screenshots in the card which show the difference in these cases: https://issues.bloomlibrary.org/youtrack/issue/BL-14362/Fix-spacing-around-cards-without-much-text Does that answer your questions?

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @StephenMcConnel)


components/language-chooser/react/language-chooser-react-mui/src/OptionCard.tsx line 51 at r1 (raw file):

Previously, nabalone (Noel) wrote…

I'm not sure if I understand the question correctly...I'll try explaining my thinking from the beginning and see if that makes sense? Every card has its own CardActionArea, which I am using only for the purpose of detecting clicks i.e. turning the card into a button. So I think we definitely want the Card and the CardActionArea to be the same size, so that we respond to clicks that are on the card and only on the card. For language cards, the parent sets a minimum height on the OptionCard, which sets the minimum height of the CardActionArea. Previously yes, some cards would omit some content and therefore be shorter, shorter than this minimum height. So in this PR I want to make the Card grow in such cases so that it is never shorter than its CardActionArea. I put some screenshots in the card which show the difference in these cases: https://issues.bloomlibrary.org/youtrack/issue/BL-14362/Fix-spacing-around-cards-without-much-text Does that answer your questions?

Steve,
I'm not sure if you saw the YouTrack card; I think it would probably answer your questions.
https://issues.bloomlibrary.org/youtrack/issue/BL-14362/Fix-spacing-around-cards-without-much-text

Copy link
Member

@StephenMcConnel StephenMcConnel left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nabalone)

@StephenMcConnel StephenMcConnel merged commit 315bff3 into main Feb 24, 2025
2 checks passed
@StephenMcConnel StephenMcConnel deleted the BL-14362_spacing_fix branch February 24, 2025 23:02
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.

3 participants