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

docs: cv-file-uploader docs enhancements #1520

Merged

Conversation

felipebritor
Copy link
Contributor

@felipebritor felipebritor commented Aug 20, 2023

Contributes to #1519

What did you do?

 Why did you do it?

  • Improve user experience / align the current documentation w/ react's

How have you tested it?

Were docs updated if needed?

  • Yes

@felipebritor
Copy link
Contributor Author

felipebritor commented Sep 20, 2023

Hi, @davidnixon . Forgive me, again, for the tardiness.

I need your feedback on a few things:

  1. I'm persisting the files uploaded in the storybook file as the storybook component is refreshed on some control changes. Unless I'm doing something wrong, as not all control changes reset the component, looks like this is the way to go.
  2. setInvalidMessage and setState are working following the example at react's storybook. Unfortunately, for clear and remove exposed methods, the native control types doesn't not offer a good option for them. There is an old issue at storybook repo requesting a button control type but not progress. One of the comments suggested a workaround which does the trick, but it's quite hacky and fragile. So I need your guidance if it makes sense to use it. We'll have to be mindful of: storybook version must be locked, keep aware that each new version update might break it and update accordingly (even making it look for a index-hash.js file does not seems stable enough but might be better than leaving it hardcoded)
    You can check it at f63ad8a.
    If you think it's better not to follow this approach, I can see other options.
    image
  3. The 'loading' state icon used together with the invalid one is broken (image 2). It's also broken in the v2 and the react version only display a single icon. Should I follow react or adjust the css?
    image

edit: this PR now is based on #1529, new commits starts at 4ddb35a

@felipebritor felipebritor force-pushed the cv-file-uploader-docs branch 2 times, most recently from dc91587 to 7d1dfe8 Compare September 25, 2023 21:37
@felipebritor
Copy link
Contributor Author

Hi, @davidnixon, just a friendly reminder about the points I need your input ☝️

@davidnixon
Copy link
Collaborator

@felipebritor oh sorry totally missed this. I will update today!

@davidnixon
Copy link
Collaborator

  1. I'm persisting the files uploaded in the storybook file as the storybook component is refreshed on some control changes. Unless I'm doing something wrong, as not all control changes reset the component, looks like this is the way to go.

This looks right to me. I know some changes cause the storybook to reload but I'm not sure why. I thought I might take a closer look at it Storybook 7.

  1. setInvalidMessage and setState are working following the example at react's storybook. ...

The clear and remove button in the storybook work correctly for me but I think you are concerned that the control type "function" is not supported?

  clear: {
    control: 'none',
    type: 'function',
    table: {
      type: { summary: '() => void' },
      category: 'exposed methods',
    },
    description: 'Clear file list',
  },
  1. The 'loading' state icon used together with the invalid one is broken ...

I think it should be like the react storybook. Only 1 icon is allowed.

The Template function is called on state/args changes, making the file
list reset. This external array will help us keep it persisted during
the user session
This is a fragile **workaround** to customize new storybook controls.

Currently, storybook lack the option to create customizable controls, as an option
for knobs. This hack allow us to define our own control types, that should be
defined at `window["STORYBOOK_CUSTOM_CONTROLS"]`, created at manager.js.

This solution, along with the button control defined at manager.js, was found in
a discussion about a new button control type:
storybookjs/storybook#11971 (comment)
The state icon displays as follows now:
- Either display one of the states & removable ("edit" state in react)
- Only show state if file is not set as invalid (unlike react, but let
it consistent)
@felipebritor
Copy link
Contributor Author

The clear and remove button in the storybook work correctly for me but I think you are concerned that the control type "function" is not supported?

function would be the type and button the control type. This control type does not exist and is set as feature request for some time now. What I did was setup a "hacky" custom button type at 1e28477.

I think it should be like the react storybook. Only 1 icon is allowed.

Done at ff4241f 👍

@felipebritor felipebritor marked this pull request as ready for review October 29, 2023 19:26
@kodiakhq kodiakhq bot merged commit 69f4d05 into carbon-design-system:main Nov 1, 2023
5 checks passed
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.

None yet

2 participants