canonical / react-components

A set of components based on Vanilla Framework
https://canonical.github.io/react-components
93 stars 51 forks source link

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

Closed lorumic closed 6 months ago

lorumic commented 6 months ago

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

webteam-app commented 6 months ago

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

mas-who commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 :+1:

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! :+1:

lorumic commented 6 months ago

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 commented 6 months ago

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 :+1:

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 commented 6 months ago

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 commented 6 months ago

LGTM :+1:

github-actions[bot] commented 6 months ago

:tada: This PR is included in version 0.50.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket: