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: make EmptyState implementation consistent with Vanilla Framework #1076

Merged
merged 5 commits into from
May 14, 2024

Conversation

vladimir-cucu
Copy link
Contributor

@vladimir-cucu vladimir-cucu commented Apr 26, 2024

Done

Fixes

Screenshots

Screenshot from 2024-04-26 23-59-55

@webteam-app
Copy link

@vladimir-cucu
Copy link
Contributor Author

Checked the changes in percy and am not sure why some of them are there. Checked locally the storybook differences between main and my branch, and couldn't replicate the changes spotted by percy, apart from the 2 changes related to EmptyState component.

@bartaz
Copy link
Member

bartaz commented Apr 26, 2024

Checked the changes in percy and am not sure why some of them are there. Checked locally the storybook differences between main and my branch, and couldn't replicate the changes spotted by percy, apart from the 2 changes related to EmptyState component.

Yes, it looks like the diff is made by older version of main branch for some reason. I'll have a look if I can restart those to get them cleaned up.

@bartaz
Copy link
Member

bartaz commented Apr 26, 2024

@vladimir-cucu I think the Percy issue is caused by the fact that your branch is likely not created from freshest main, so you are couple commits behind, making your snapshots show older versions of some components that changed in main branch.

Could you fetch and rebase on top of fresh main and push again? This should hopefully fix that.

Copy link
Member

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

height: "2.5rem",
width: "2.5rem",
height: "10rem",
width: "10rem",
Copy link
Member

Choose a reason for hiding this comment

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

Icons shouldn't be used this way. They were not designed to be scaled that much up (and weird things may start happening).

This one for example doesn't scale on small screens due to missing max-width that we have on img:
image

Ideally, this should only be used with images. Would be good not to document an example with a bad pattern like this.

Maybe let's use this one from assets: https://assets.ubuntu.com/v1/c17e0d92-container.svg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment! Didn't know about this. The default example has the exact same issue with scaling on small screens. I've used the image from the assets for the default example as well.

@vladimir-cucu
Copy link
Contributor Author

@vladimir-cucu I think the Percy issue is caused by the fact that your branch is likely not created from freshest main, so you are couple commits behind, making your snapshots show older versions of some components that changed in main branch.

Could you fetch and rebase on top of fresh main and push again? This should hopefully fix that.

@bartaz After rebase on top of main with latest changes and 2 more placeholder PRs to restart percy, it finally shows the expected changes!

Copy link
Member

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@vladimir-cucu vladimir-cucu merged commit 407add1 into canonical:main May 14, 2024
13 checks passed
Copy link

🎉 This PR is included in version 0.53.1 🎉

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