-
Notifications
You must be signed in to change notification settings - Fork 16
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
[Files] Breadcrumb enhancement for shared folders #2195
Conversation
Your Render PR Server URL is https://chainsafe-components-stage-pr-2195.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-cam985k41ls7aejuq120. |
Your Render PR Server URL is https://files-ui-stage-pr-2195.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-cam986s41ls7aejuq16g. |
Your Render PR Server URL is https://storage-ui-stage-pr-2195.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-cam988441ls7aejuq1o0. |
Tests failing is unrelated, I opened #2198 edit: fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: I put the review for other PR, my bad
@Tbaut Looking good in Chrome! I need to test in other browsers too and mobile, but seems good so far. I only have one question: If I understood correctly, based on #1284 (comment) comment, the last folder name should be complete and not with elipsis: Maybe we need more clarification there. |
Ah I didn't take this into account here. I didn't touch this part and just went for the Breadcrumb enhancement for shared folders and the root. Any folder with a too long name will have ellipsis as before. I guess we can tackle that in another issue if that's really an issue (which I don't really think it is). |
Yes, make sense, I can create another ticket and we can discuss it tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, this is an issue with the svg as well. Added a quick fix for this particular issue, but the svg still needs attention. |
The icon is now fixed and it scales correctly 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all working perfect now! awesome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works great 🎉
I missed that sorry. So the home icon should be 20px size, and this shouldn't have changed (in the app, I didn't check storybook). |
closes #1284
Submission checklist:
Layout
Theme