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

fix(flex): in breakpointMods set breakpoint as optional and add enums #3258

Merged
merged 1 commit into from Nov 19, 2019

Conversation

@boaz0
Copy link
Member

boaz0 commented Nov 3, 2019

What:

closes #3244
closes #3248

  • Set breakpoint as optional in breakpointMd
  • Add FlexModifiers and FlexItemModifiers
  • Update examples
Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 3, 2019

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

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 3, 2019

Codecov Report

Merging #3258 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3258   +/-   ##
=======================================
  Coverage   67.43%   67.43%           
=======================================
  Files         892      892           
  Lines       24869    24869           
  Branches     2140     2140           
=======================================
  Hits        16770    16770           
  Misses       7094     7094           
  Partials     1005     1005
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.3% <ø> (ø) ⬆️
#patternfly4 64.75% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c8ea98...ef45062. Read the comment docs.

@rebeccaalpert

This comment has been minimized.

Copy link
Member

rebeccaalpert commented Nov 6, 2019

@tlabaj wasn't there a discussion about how we wouldn't use enums in TypeScript components? Has that changed?

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Nov 7, 2019

@rebeccaalpert I didn't know that you're not going to use enums.
What should I use instead? Because if I am using literal types, passing a string will show a typescript error unless I will use the as keyword.

@rebeccaalpert

This comment has been minimized.

Copy link
Member

rebeccaalpert commented Nov 7, 2019

@boaz0 - It sounds like enums are ok. (Wanted to check with more active PatternFly devs.) Back when we were converting all the components to TypeScript there had been some back-and-forth over declaring everything as strings vs. using enums.

Copy link
Member

rebeccaalpert left a comment

Looks good to me.

@tlabaj
tlabaj approved these changes Nov 19, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 285ea43 into patternfly:master Nov 19, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@boaz0 boaz0 deleted the boaz0:closes_3244 branch Nov 19, 2019
'align-self-flex-stretch' = 'align-self-flex-stretch',
'justify-content-flex-end' = 'justify-content-flex-end',
'justify-content-center' = 'justify-content-center',
'justify-content-flex-space-between' = 'justify-content-flex-space-between',

This comment has been minimized.

Copy link
@sahil143

sahil143 Nov 20, 2019

Member

@boaz0 @tlabaj Not sure if this value is correct. Docs example says to use justify-content-space-between https://github.com/patternfly/patternfly-react/pull/3258/files#diff-fee6334c94230e770989b535c35f87f7R428
Also, justify-content-flex-space-between doesn't work when this is used an undefined class is applied to the element.

This comment has been minimized.

Copy link
@boaz0

boaz0 Nov 20, 2019

Author Member

😨 ho snap! I see this now. It looks like the Modifier type was in correct to begin with. It didn't have justify-content-space-between and the flex.css doesn't have justify-content-space-between but justify-content-space-between.

My mistake for not paying attention. I will suggest a fix

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