FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(flex): in breakpointMods set breakpoint as optional and add enums by boaz0 · Pull Request #3258 · patternfly/patternfly-react · GitHub

/ patternfly-react Public

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

Merged
tlabaj merged 1 commit into
patternfly:masterfrom
boaz0:closes_3244
Nov 19, 2019
ConversationCommits (1)ChecksFiles changed
Merged

fix(flex): in breakpointMods set breakpoint as optional and add enums#3258
tlabaj merged 1 commit into
patternfly:masterfrom
boaz0:closes_3244

Conversation

boaz0 commented Nov 3, 2019

Copy link
Copy Markdown
Member

What:

closes #3244
closes #3248

  • Set breakpoint as optional in breakpointMd
  • Add FlexModifiers and FlexItemModifiers
  • Update examples

fix(flex): in breakpointMods set breakpoint as optional and add enums
ef45062 
Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>

patternfly-build commented Nov 3, 2019

Copy link
Copy Markdown
Collaborator

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

codecov-io commented Nov 3, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

@@           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.

tlabaj requested a review from rebeccaalpert November 5, 2019 17:44

rebeccaalpert commented Nov 6, 2019

Copy link
Copy Markdown
Member

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

boaz0 commented Nov 7, 2019

Copy link
Copy Markdown
Member Author

@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 commented Nov 7, 2019

Copy link
Copy Markdown
Member

@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.

rebeccaalpert approved these changes Nov 7, 2019
View reviewed changes

rebeccaalpert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me.

tlabaj approved these changes Nov 19, 2019
View reviewed changes

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

tlabaj merged commit 285ea43 into patternfly:master Nov 19, 2019
boaz0 deleted the closes_3244 branch November 19, 2019 18:30
sahil143 reviewed Nov 20, 2019
View reviewed changes
Comment thread packages/patternfly-4/react-core/src/layouts/Flex/FlexUtils.tsx
'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',

sahil143 Nov 20, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

boaz0 Nov 20, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

😨 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

boaz0 mentioned this pull request Nov 20, 2019
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

rebeccaalpert rebeccaalpert approved these changes
+3 more reviewers
sahil143 sahil143 left review comments
dlabrecq dlabrecq approved these changes
tlabaj tlabaj approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

rebeccaalpert

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Flex component TypeScript error PF4 Layout Flex: Typing improvements for breakpointMods

7 participants

Back | FazBrowse Home | New Git URL