-
Notifications
You must be signed in to change notification settings - Fork 15
fix: 🐛 handle long filenames #754
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
||
| const Wrapper = styled.div` | ||
| width: 100%; | ||
| @media (min-width: 768px) { |
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.
We could use the breakpoints form our list of tokens
| state, | ||
| className, | ||
| size, | ||
| displayAs = "block", |
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.
@punkbit Why do we need the displayAs
Also here we are using block and inline but the createSVGWrapper uses flex and inline-flex? May I know the reason behind this
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.
Best to check the current version in the PR description, e.g. is squashed. This change is to keep retroactive support while addressing support for icons as an inline element, e.g. along text.
| $size === "sm" | ||
| ? theme.click.fileUpload.sm.space.gap | ||
| : theme.click.fileUpload.md.space.gap}; | ||
| gap: 0.5rem; |
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.
@gjones Shouldn't we use tokens instead of the hardcoded values here
| margin-top: auto; | ||
| `; | ||
|
|
||
| InlineIcon.defaultProps = { |
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.
I don't think we need to add defaultProps like using proptypes anymore as in react 19 it is deprecated and we would soon move to react 19 as well
Why?
Bucky asked to complete a project assignment fixing a ClickHouse UI issue on the FileUpload component.
The assignment requires showing as much of the filename as possible with good UX across various screen widths.
Must submit the solution, including documentation of thought process and experimentation, which I've decided to keep it here in the PR description for easy access.
How?
Demo?
Before:
clickhouse-current-version-broken-examples-walkthrough.mov
default-flyout-width-transition-causes-component-shifting.mp4
💡 Since the success state doesn't display a text label like 'File uploaded,' it would be more consistent to use an error icon instead of text for failures. If the 'Failed upload' text is required, can consider relocating it to avoid lengthening the already long filename line. Alternatively, the icon alone would likely be sufficient for most users without the accompanying text. Thus, the icon must be a inline element to prevent detachment and keeping context, e.g. see gap.
After:
PR file upload version proposes the following changes:
clickhouse-pr-fix-walkthrough.mov
👌 Reveal the complete filename on hover in large containers. Skip smaller container/viewports for the moment. This pattern can be found in OS such as macOS, e.g. hover a long filename in the file explorer, or in the hash strings found in blockchain-like UIs.
clickhouse-upload-success-hover-reveal.mp4
It can be useful when errors are found, providing context to the user
clickhouse-upload-failure-reveal-retry.mp4
👌 Shows the file uploader in different container sizes. It uses the viewport switch for a quick demo, styles are not based in media queries viewports, but declared over container width breakpoints.
clickhouse-container-breakpoints-resize.mp4
Here's demonstrated with free width flow:
clickhouse-responsive.mp4
👌 Here's demonstrated that the default flyout "width transition" causes element position shifting. While this might be expected or a personal preference, a quick demo shows a very similar transition approach that handles it more gracefully without causing position shifting.
default-flyout-width-transition-causes-component-shifting.mp4
💡 Truncate filenames by shortening the middle revealing critical parts
Assume you have:
In the current faulty version you'd get something like:
Notice that the first two filenames, when presented truncated, have the same shortened name, making it hard to differentiate.
In the PR proposed version you'd find easier to identify files if these are named in a maintainable way:
Notice that the first and last digits help identify the file more concisely, allowing for a shorter length.
Find two new stories in the storybook playground as follows: