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

fix: hide description when width is below the small breakpoint #1043

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

lorumic
Copy link
Contributor

@lorumic lorumic commented Feb 9, 2024

Done

Even in its shortened version, the description for the table pagination goes new line multiple times on small widths:

image

To solve this, the description element is now hidden when width is below the small breakpoint.

image

QA

Storybook

To see rendered examples of all react-components, run:

yarn start

QA in your project

from react-components run:

yarn build
npm pack

Install the resulting tarball in your project with:

yarn add <path-to-tarball>

QA steps

  • On any storybook example for the table pagination, resize the window so that the width is lower than 620px (small breakpoint).
  • Check that the description is hidden.

@webteam-app
Copy link

Demo starting at https://react-components-1043.demos.haus

@lorumic lorumic changed the title Hide description when width is below the small breakpoint fix: hide description when width is below the small breakpoint Feb 9, 2024
@mas-who
Copy link
Contributor

mas-who commented Feb 12, 2024

Not sure why, seems like the description is hidden when screen width is 850px instead of 620px for some reason, not sure why since I see the media query is applied for 620px. Also, should we also adjust the container to justify content to flex-end when the description is hidden to make sure the controls are always right aligned?

Screenshot from 2024-02-12 10-44-53

@lorumic
Copy link
Contributor Author

lorumic commented Feb 12, 2024

Not sure why, seems like the description is hidden when screen width is 850px instead of 620px for some reason, not sure why since I see the media query is applied for 620px.

I think this happens because the storybook example is placed inside an iframe, so what you should consider is not your window's width, but the iframe width.

Also, should we also adjust the container to justify content to flex-end when the description is hidden to make sure the controls are always right aligned?

Sure, will push a fix for this shortly.

@mas-who
Copy link
Contributor

mas-who commented Feb 12, 2024

Not sure why, seems like the description is hidden when screen width is 850px instead of 620px for some reason, not sure why since I see the media query is applied for 620px.

I think this happens because the storybook example is placed inside an iframe, so what you should consider is not your window's width, but the iframe width.

Yes, that's true, sorry for the confusion 👍

Also, should we also adjust the container to justify content to flex-end when the description is hidden to make sure the controls are always right aligned?

Sure, will push a fix for this shortly.

Thanks! 👍

@lorumic lorumic force-pushed the fix-tablepagination-small-width branch from c986a49 to 00da44a Compare February 12, 2024 10:08
@lorumic
Copy link
Contributor Author

lorumic commented Feb 12, 2024

Also, should we also adjust the container to justify content to flex-end when the description is hidden to make sure the controls are always right aligned?

Fixed this. Also, I changed the margin slightly to make it more uniform and balanced between top and bottom. I think the "top-only" margin comes from the implementation in LXD UI, where the table pagination is above the table, and the spacing between it and the table itself is handled by some other element (the table itself or its wrapper maybe?), but for the general case I think it would be better to have a balanced margin, so that it works well also when the table pagination is below the table, perhaps at the bottom of the page constraining the height of a ScrollableTable above it - which is our use case in Anbox. :)

@mas-who
Copy link
Contributor

mas-who commented Feb 12, 2024

Also, should we also adjust the container to justify content to flex-end when the description is hidden to make sure the controls are always right aligned?

Fixed this. Also, I changed the margin slightly to make it more uniform and balanced between top and bottom. I think the "top-only" margin comes from the implementation in LXD UI, where the table pagination is above the table, and the spacing between it and the table itself is handled by some other element (the table itself or its wrapper maybe?), but for the general case I think it would be better to have a balanced margin, so that it works well also when the table pagination is below the table, perhaps at the bottom of the page constraining the height of a ScrollableTable above it - which is our use case in Anbox. :)

Yeah that sounds good to me, I think project specific styling requirements should be dealt with in the project itself, making the spacing general for the component here makes sense 👍

The change isn't reflecting on demo server though, I see it's still got the margin-top: 1.2rem applied. Can we do a force push maybe to just trigger another build? I'd like to just have a look at the component

@lorumic
Copy link
Contributor Author

lorumic commented Feb 12, 2024

The change isn't reflecting on demo server though, I see it's still got the margin-top: 1.2rem applied. Can we do a force push maybe to just trigger another build? I'd like to just have a look at the component

I think we sorted out that this was a caching issue, let me know if you can see the changes now.

@mas-who
Copy link
Contributor

mas-who commented Feb 12, 2024

LGTM 👍

@lorumic lorumic merged commit 829ba13 into canonical:main Feb 12, 2024
6 checks passed
@lorumic lorumic deleted the fix-tablepagination-small-width branch February 12, 2024 15:03
Copy link

🎉 This PR is included in version 0.50.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants