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

feat: TET-853 add FileItem component #143

Merged
merged 13 commits into from
May 16, 2024
Merged

Conversation

mjamrozekvl
Copy link
Contributor

@mjamrozekvl mjamrozekvl commented May 13, 2024

What did you implement?

FileUploader component has been implemented. This is one of building blocks of more complex component: FileUploader

  • I decided to change a little bit how our stories should look like. I changed number of examples from 4 to 2 in single row:
    image
    because of our width limitation to 888px.
  • Mentioned component has multiple properties and initial implementation caused a lot of control flow statements in FileItem component. Because of readability I isolated 2 smaller components: CompressedVariant and ExtendedVariant. I discovered that compressed version of FileItem:
    • doesn't have file icon or thumbnail
    • doesn't have time left text
    • structure of internal blocks is different because of size, thumbnail, different location of file size, progress bar
  • I also discovered that file item icon is not present in our database of icons and we only have icon for PDF file format. I decided to use existing icon (20-file) instead of icon from Figma:
    image
  • By design we try to set as much properties as we can to optional and fallback it with default values. I had to use those fallbacks in different places so I thought that defining a type Fallback inside our *.props.ts file and create a constant where all possible fallbacks are hardcoded is a good practice and can be imported in component file, styles builder etc. without need for duplication of our default values.

How it might be Tested?

Run storybook

yarn storybook

Checklist:

@mjamrozekvl mjamrozekvl self-assigned this May 13, 2024
@mjamrozekvl mjamrozekvl marked this pull request as draft May 13, 2024 06:33
@mjamrozekvl mjamrozekvl marked this pull request as ready for review May 13, 2024 07:36
@mjamrozekvl mjamrozekvl changed the title [WIP] feat: TET-853 add FileItem component feat: TET-853 add FileItem component May 13, 2024
@adrian-potepa
Copy link
Collaborator

Component with replaceable state should has 40px height:
image

@adrian-potepa
Copy link
Collaborator

adrian-potepa commented May 14, 2024

Dot and spacing between siblings elements are wrong.

You can place the dot as JSX svg element ;)

Zrzut ekranu 2024-05-14 o 19 11 54

Copy link
Collaborator

@adrian-potepa adrian-potepa left a comment

Choose a reason for hiding this comment

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

Nice work! It needs only few small things to do :)

@mjamrozekvl
Copy link
Contributor Author

mjamrozekvl commented May 15, 2024

Component with replaceable state should has 40px height: image

Yes. Good catch. I set button paddings to 0px using custom styles and now it is 40px height (also for alert version because it is also a little bit expanded).

@mjamrozekvl
Copy link
Contributor Author

mjamrozekvl commented May 15, 2024

Dot and spacing between siblings elements are wrong.

You can place the dot as JSX svg element ;)

Zrzut ekranu 2024-05-14 o 19 11 54

I made it using styles, wdyt?

    dot: {
      display: 'inline-block',
      mx: '$space-component-gap-small',
      w: '2px',
      h: '2px',
      backgroundColor: '$color-content-secondary',
      verticalAlign: 'middle',
    },

I thought about token instead of ordinary 2px value but I couldn't find any appropriate, only $border-width-200 has 2px.

BTW according to Figma we should have margin: $space-component-gap-small but maybe $space-component-gap-xSmall would be better because of spacing inside letters?

@adrian-potepa
Copy link
Collaborator

BTW according to Figma we should have margin: $space-component-gap-small but maybe $space-component-gap-xSmall would be better because of spacing inside letters?

If you style container of HelperText, dot and text as flex you will get good render of spacing ;)

<tet.div display="flex" alignItems="center" gap="$space-component-gap-small">
   <HelperText />
   <Dot />
   <span>some text</span>
</tet.div/>

In that case there is no need to add mx: '$space-component-gap-small' in dot object.

@mjamrozekvl
Copy link
Contributor Author

mjamrozekvl commented May 16, 2024

BTW according to Figma we should have margin: $space-component-gap-small but maybe $space-component-gap-xSmall would be better because of spacing inside letters?

If you style container of HelperText, dot and text as flex you will get good render of spacing ;)

<tet.div display="flex" alignItems="center" gap="$space-component-gap-small">
   <HelperText />
   <Dot />
   <span>some text</span>
</tet.div/>

In that case there is no need to add mx: '$space-component-gap-small' in dot object.

Initially I thought about it and I was hesitant against overengineering but definitely it ensures accuracy so I introduced it :-)

Copy link
Collaborator

@adrian-potepa adrian-potepa left a comment

Choose a reason for hiding this comment

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

🎊

@mjamrozekvl mjamrozekvl merged commit 9f50c5e into main May 16, 2024
1 check passed
@mjamrozekvl mjamrozekvl deleted the feat/TET-853-file-item branch May 16, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants