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

RAC-225: DSM checkbox #12686

Merged
merged 23 commits into from Sep 11, 2020
Merged

RAC-225: DSM checkbox #12686

merged 23 commits into from Sep 11, 2020

Conversation

StevenVAIDIE
Copy link
Collaborator

@StevenVAIDIE StevenVAIDIE commented Sep 7, 2020

Description (for Contributor and Core Developer)
This PR contain :

Definition Of Done (for Core Developer only)

Q A
Added Specs Todo
Added legacy Behats Todo
Added acceptance tests Todo
Added integration tests Todo
Changelog updated Todo
Review and 2 GTM Todo
Micro Demo to the PO (Story only) Todo
Migration script -
Tech Doc -

Todo: Pending / Work in progress
OK: Done / Validated
-: Not needed

@StevenVAIDIE StevenVAIDIE changed the base branch from master to dsm September 7, 2020 09:07
@StevenVAIDIE StevenVAIDIE changed the title Rac 225 RAC-225: DSM checkbox Sep 7, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2020

A new version of the staging env has been deployed 🎉
Check it out here: https://samirboulil.github.io/dsm/12686/

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2020

A new version of the staging env has been deployed 🎉
Check it out here: https://samirboulil.github.io/dsm/12686/

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2020

Your visual regression tests are not passing.
Here is a pull request to fix them: #12690

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2020

A new version of the staging env has been deployed 🎉
Check it out here: https://samirboulil.github.io/dsm/12686/

Copy link
Contributor

@juliensnz juliensnz left a comment

Choose a reason for hiding this comment

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

GTM after fix

Also:

akeneo-design-system/src/components/Checkbox/Checkbox.tsx Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2020

A new version of the staging env has been deployed 🎉
Check it out here: https://samirboulil.github.io/dsm/12686/

* @TODO use grey140 instead of #11324d
*/

const CheckboxContainer = styled.div < {checked: boolean, readOnly: boolean } > `
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use the HTML <input type="checkbox"> element for semantical and accessibility purposes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I done this in a previous version this but with "undetermined" state i don't know how to do it

Copy link
Contributor

@SamirBoulil SamirBoulil left a comment

Choose a reason for hiding this comment

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

If we are to merge this PR, shouldn’t we need to seek approval from Stephan ?

Copy link
Contributor

@SamirBoulil SamirBoulil left a comment

Choose a reason for hiding this comment

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

Niiiice !
GTM after Steph’ has reviewed it!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2020

Your visual regression tests are not passing.
Here is a pull request to fix them: #12695

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2020

Your visual regression tests are not passing.
Here is a pull request to fix them: #12695

@github-actions
Copy link
Contributor

Your visual regression tests are not passing.
Here is a pull request to fix them: #12711

Co-authored-by: StevenVAIDIE <StevenVAIDIE@users.noreply.github.com>
@github-actions
Copy link
Contributor

A new version of the staging env has been deployed 🎉
Check it out here: https://samirboulil.github.io/dsm/12686/

@github-actions
Copy link
Contributor

Your visual regression tests are not passing.
Here is a pull request to fix them: #12719

Co-authored-by: StevenVAIDIE <StevenVAIDIE@users.noreply.github.com>
@github-actions
Copy link
Contributor

A new version of the staging env has been deployed 🎉
Check it out here: https://samirboulil.github.io/dsm/12686/

1 similar comment
@github-actions
Copy link
Contributor

A new version of the staging env has been deployed 🎉
Check it out here: https://samirboulil.github.io/dsm/12686/

@github-actions
Copy link
Contributor

A new version of the staging env has been deployed 🎉
Check it out here: https://samirboulil.github.io/dsm/12686/

const checkbox = getByText('Checkbox');
fireEvent.click(checkbox);

expect(onChange).not.toBeCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we check the checkbox is still checked ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For me, we didn't need to check if the checkbox is checked because it's a controlled component but I don't know @ValentinMumble what is your opinion ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, as it is a controlled component there shouldn't be any internal logic that could change the checked prop so not really needed for me as well but as you wish

@github-actions
Copy link
Contributor

Your visual regression tests are not passing.
Here is a pull request to fix them: #12720

Co-authored-by: StevenVAIDIE <StevenVAIDIE@users.noreply.github.com>
@github-actions
Copy link
Contributor

A new version of the staging env has been deployed 🎉
Check it out here: https://samirboulil.github.io/dsm/12686/

@StevenVAIDIE StevenVAIDIE merged commit 29ec1b5 into dsm Sep 11, 2020
@StevenVAIDIE StevenVAIDIE deleted the RAC-225 branch September 11, 2020 13:30
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.

None yet

4 participants