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(checkbox): implement indeterminate state #16951

Merged

Conversation

3 participants
@simonhaenisch
Copy link
Contributor

commented Jan 3, 2019

Short description of what this resolves:

Implements an indeterminate state for checkboxes.

Changes proposed in this pull request:

This adds an indeterminate prop to the ion-checkbox component, which visually renders the checkbox with a dash to indicate an indeterminate state.

MD iOS
indeterminate-checkbox-md indeterminate-checkbox-ios

For MD styles I followed Material specs (see section "Parent and child checkboxes") and the Material Components for Web implementation to implement the dash according to the specs. For iOS styles I had a look at https://github.com/Marxon13/M13Checkbox.

Should I add an E2E test for the most common use case of it, which is a nested tree of checkboxes? That's why I'm submitting this anyway, because we need it for our work.

One change I am not totally sure about is that I had to remove position: static for checkboxes within items. Not sure why it was there in the first place because it seems to work fine with position: relative, however I assume there was a reason for this?

Ionic Version: 4.0.0-rc.0

Closes: #16943

feat(checkbox): implement indeterminate state
This adds an `indeterminate` prop to the `ion-checkbox` component,
which visually renders the checkbox with a dash to indicate an
indeterminate state.

Closes #16943.

simonhaenisch added some commits Jan 4, 2019

@simonhaenisch

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

@brandyscarney could you have a look at this please? We want to use this in our app and would prefer ion-checkbox supporting this rather than having to create our own checkbox component.

I can work on getting this PR ready if any changes are required.

@brandyscarney brandyscarney added this to In progress 🤺 in Ionic Core via automation Feb 13, 2019

@brandyscarney brandyscarney moved this from In progress 🤺 to Needs review 🤔 in Ionic Core Feb 21, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Thanks for the PR! I plan to review this soon. Sorry for the delay. 🙂

@liamdebeasi
Copy link
Member

left a comment

The - isn't aligned properly on Chrome and Safari. Looks like the margin is incorrect
image
image

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Also, we are planning on removing the preview folders, so would you be able to add your tests to the basic test? Thank you! 🙂

@simonhaenisch

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

ok will do that later... my screenshots were taken in chrome though, so not sure why it wouldn’t work for you? anyway i’ll make sure it’ll work in safari and firefox as well.

brandyscarney and others added some commits Feb 26, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

This is such a great PR! Nice work. I am going to take a look at this now and make some changes to the tests. Just letting you know so we don't do twice the work. 🙂

brandyscarney added some commits Feb 28, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

I was actually seeing the same thing as Liam in Chrome, so I modified it to use an svg like the checkmark icon which eliminates a lot of the CSS. This may have broken some of the functionality, I am still looking into it and going to work on adding the parent/children test case next.

screen shot 2019-02-28 at 12 59 31 pm

brandyscarney added some commits Feb 28, 2019

I updated the code that was reviewed

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

I made some changes to the CSS to add back some things that were removed. I believe position: static was added so that checkbox wouldn't have problems if it were in an item with a toggle or something similar, but I can't recall the exact reason.

I added a native indeterminate checkbox example to the test here: http://localhost:3333/src/components/checkbox/test/indeterminate

This shows that our determinate checkboxes are behaving the same as native. I also added a parent/child example and it seems to be working as expected.

Please let me know if you're seeing any problems with my changes, otherwise it should be good to go. Thank you!!

@liamdebeasi
Copy link
Member

left a comment

This looks great! 🎉

liamdebeasi and others added some commits Mar 1, 2019

@simonhaenisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Yup looks great, thanks for your work!

@brandyscarney brandyscarney merged commit c641ae1 into ionic-team:master Mar 4, 2019

1 check passed

build Workflow: build
Details

Ionic Core automation moved this from Needs review 🤔 to Done 🎉 Mar 4, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Thank you 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.