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

Fixes bug in the EuiCheckboxGroup demo page and add disabled to TS defs #2545

Merged
merged 3 commits into from
Nov 19, 2019

Conversation

ffknob
Copy link
Contributor

@ffknob ffknob commented Nov 18, 2019

Related to #2538

Fixes bug in the EuiCheckboxGroup demo page in which both enabled and disabled groups were using the same ids for their checkbox input components, which caused the disabled ones to be checkable

Checklist

  • [ ] Checked in dark mode
  • [ ] Checked in mobile
  • [ ] Checked in IE11 and Firefox
  • [ ] Props have proper autodocs
  • Added documentation examples
  • [ ] Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

…nd disabled groups were using the same ids for their checkbox input components, which caused the disabled ones to be checkable
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@ffknob ffknob changed the title Fixex bug in the EuiCheckboxGroup demo page in which both enabled a… Fixes bug in the EuiCheckboxGroup demo page in which both enabled a… Nov 18, 2019
src-docs/src/views/form_controls/checkbox_group.js Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@chandlerprall chandlerprall self-requested a review November 19, 2019 01:45
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks for that change @ffknob .

One of the things we use the documentation page for is to visually test the different states of a component. With all the disabled checkboxes being unchecked now, we can't visually see what the state of a disabled and checked checkbox would look like. That's most likely the reason we kept the id's the same which is simply to check the look of this state.

Could you look into creating a different checkboxIdToSelectedMap state for the disabled options and by default set one of them to checked?

@ffknob
Copy link
Contributor Author

ffknob commented Nov 19, 2019

Thanks for that change @ffknob .

One of the things we use the documentation page for is to visually test the different states of a component. With all the disabled checkboxes being unchecked now, we can't visually see what the state of a disabled and checked checkbox would look like. That's most likely the reason we kept the id's the same which is simply to check the look of this state.

Could you look into creating a different checkboxIdToSelectedMap state for the disabled options and by default set one of them to checked?

How about that?

Bildschirmfoto vom 2019-11-19 11-00-23

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

That's great, thank you!

@cchaos
Copy link
Contributor

cchaos commented Nov 19, 2019

Jenkins, test this

@cchaos cchaos changed the title Fixes bug in the EuiCheckboxGroup demo page in which both enabled a… Fixes bug in the EuiCheckboxGroup demo page and add disabled to TS defs Nov 19, 2019
@cchaos cchaos merged commit da12252 into elastic:master Nov 19, 2019
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.

4 participants