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
merged 26 commits into from
Mar 4, 2019
Merged

feat(checkbox): implement indeterminate state #16951

merged 26 commits into from
Mar 4, 2019

Conversation

simonhaenisch
Copy link
Contributor

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

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.
@ionitron-bot ionitron-bot bot added package: angular @ionic/angular package package: core @ionic/core package labels Jan 3, 2019
@simonhaenisch
Copy link
Contributor Author

@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
Copy link
Member

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

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

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

@liamdebeasi
Copy link
Contributor

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
Copy link
Contributor Author

simonhaenisch 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
Copy link
Member

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
Copy link
Member

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 brandyscarney dismissed liamdebeasi’s stale review February 28, 2019 22:17

I updated the code that was reviewed

@brandyscarney
Copy link
Member

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!!

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

This looks great! 🎉

@simonhaenisch
Copy link
Contributor Author

Yup looks great, thanks for your work!

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

Thank you 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants