Skip to content

Conversation

@guillaumevincent
Copy link
Contributor

fix #728

@guillaumevincent
Copy link
Contributor Author

guillaumevincent commented Oct 15, 2018

I'm waiting for the switch component to be released in patternfly-core

@patternfly-build
Copy link
Collaborator

PatternFly-React preview: https://761-pr-patternfly-react-patternfly.surge.sh

@amarie401 amarie401 added the PF4 label Oct 15, 2018
@amarie401 amarie401 requested review from jschuler and tlabaj October 15, 2018 13:39
@amarie401
Copy link
Contributor

cc: @maryshak1996

@maryshak1996
Copy link

@guillaumevincent on my end, it isn't showing the element (must not be available in core yet), so I'll check back to review visuals frequently to keep things moving :)

@LHinson
Copy link
Member

LHinson commented Oct 15, 2018

This was a convo on slack but adding here to make sure everyone saw....

the pf-next commit message didn't include a space after fix(topic):asdfdsfads therefore travis is saying there are no changes that should be included and it didn't kick off a release.

Once another proper commit is made to core, the switch should be available with that release. @dgutride will be adding stricter rules to help enforce proper commit messages and improve the releases.

@coveralls
Copy link

coveralls commented Oct 16, 2018

Pull Request Test Coverage Report for Build 3233

  • 8 of 10 (80.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 81.192%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/patternfly-4/react-core/src/components/Switch/Switch.js 8 10 80.0%
Totals Coverage Status
Change from base Build 3219: 0.006%
Covered Lines: 3795
Relevant Lines: 4406

💛 - Coveralls

@guillaumevincent
Copy link
Contributor Author

I don't know why PatternFly-React preview is not updated, but this PR is ready for a review

Copy link
Member

@christiemolloy christiemolloy left a comment

Choose a reason for hiding this comment

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

@guillaumevincent This implementation is great! The CSS aligns with what we have in pf-next. Only thing, I see there is an "Uncontrolled Switch" in react but not in pf-next, was this intentional? I assume that these should be synonymous, but correct me if I'm wrong :)

@guillaumevincent
Copy link
Contributor Author

@christiemolloy I copy the Checkbox component here
Tell me if you want me to copy the pf-next implementation

@jschuler I agree for the event props should be first
Can we create an issue for this one? and fix it in another PR?

@guillaumevincent
Copy link
Contributor Author

@jschuler just for your information you can propose change directly in the PR
See https://twitter.com/github/status/1054794897448939520

@guillaumevincent
Copy link
Contributor Author

@jgiardino I added a PR on pf-core
I will update this PR on monday
Thanks

dlabaj
dlabaj previously approved these changes Oct 26, 2018
@mcarrano
Copy link
Member

That's fair @guillaumevincent . I will consult with Visuals and open a new issue if we decide that the Switch labeling should be updated in both places.

I was going to mark this as "UX Approved" but looks like I don't have edit rights on this repo. @LHinson is that something you can enable?

@guillaumevincent
Copy link
Contributor Author

@mcarrano I fix the label in pf-next
I propose to wait for patternfly/patternfly#877 to be merged
wdyt?

@mcarrano
Copy link
Member

Yes, makes sense @guillaumevincent .

@rachael-phillips
Copy link
Contributor

@dlabaj @mcarrano @tlabaj patternfly/patternfly#877 merged. Is this one ready for merge?

@mcarrano
Copy link
Member

mcarrano commented Nov 8, 2018

@rachael-phillips we had some discussion about the Switch component in the Accessibility meeting today and may be making some changes on the Core side. Opened a new issue here: patternfly/patternfly#914 We could merge this and create a new React issue or hold on this until core changes are made.

@dgutride
Copy link
Member

dgutride commented Nov 9, 2018

We should let this in - @guillaumevincent - please make sure the build is passing and then it can be merged.

@rachael-phillips
Copy link
Contributor

rachael-phillips commented Nov 9, 2018 via email

@guillaumevincent guillaumevincent force-pushed the master branch 2 times, most recently from 49b8d32 to 1d87c60 Compare November 13, 2018 15:20
dgutride
dgutride previously approved these changes Nov 14, 2018
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@christiemolloy
Copy link
Member

@jgiardino do you know the status of the switch component. Was @mceledonia going to make visual updates? In this react example the text is "on/off" should we keep this wording or switch to "label" like we have in core.

@jgiardino
Copy link
Contributor

If this is ready to merge then I'd vote to handle the visual updates (patternfly/patternfly#914) in a later PR since they haven't been implemented in core yet.

Let's keep On/Off here. There's also an issue in core to handle toggling the label (patternfly/patternfly#915). I suggested we change the label to "On" and "Off" in that issue. So let's keep "On"/"Off" in this one, and core will match when that issue is done.

@jschuler
Copy link
Collaborator

@jgiardino Sounds good, looks like this is good to merge and there are some followup items in the 2 issues you referenced.

@jschuler jschuler merged commit ef13fd2 into patternfly:master Nov 15, 2018
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.

PF4 Switch Component