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

Controls: Add id to setter button for undefined values #15729

Merged

Conversation

revik
Copy link
Contributor

@revik revik commented Jul 31, 2021

Issue: #15690

What I did

added getControlSetterButtonId function to calculate id of setter button for undefined control values

How to test

added a test for the getControlSetterButtonId function

  • Is this testable with Jest or Chromatic screenshots? yes
  • Does this need a new example in the kitchen sink apps? no
  • Does this need an update to the documentation? no

@nx-cloud
Copy link

nx-cloud bot commented Jul 31, 2021

Nx Cloud Report

CI ran the following commands for commit 11e3cab. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks @revik! This is looking pretty good. However, I noticed the following controls are missing: Color, Date, Files, Options, Range, Checkbox, Radio, Select. If we're adding test IDs to some of the controls we should add to all. WDYT?

@revik
Copy link
Contributor Author

revik commented Aug 2, 2021

Thanks @revik! This is looking pretty good. However, I noticed the following controls are missing: Color, Date, Files, Options, Range, Checkbox, Radio, Select. If we're adding test IDs to some of the controls we should add to all. WDYT?

@shilman those controls do not display the 'Set' button when no value is sent to them. They all display the control without a specific set value. IMHO - aligning all controls to have the 'Set' button when there is not value set IS the best choice..
Do you want me to try and add it to all of the above controls?

Should this added change be in this PR or in a different one?

@shilman
Copy link
Member

shilman commented Aug 2, 2021

Aha, my bad @revik -- I totally missed that. I agree that we should have a unified UI for unset values. I know @tmeasday and @domyen didn't agree before, but I think everybody agrees now. It would be great if you could also fix that in this PR! 🙏

@shilman shilman changed the title add id to control setter button for undefined control values Controls: Add id to setter button for undefined values Aug 17, 2021
@shilman
Copy link
Member

shilman commented Aug 17, 2021

Hi @revik I'm going to merge this in the interest of moving things forward. If you still have any interest, could you please pick up the other changes in a new PR?

@shilman
Copy link
Member

shilman commented Aug 17, 2021

Merging despite failing tests. Those tests are already failing on next

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