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(list): add the ordered variant #3363

Merged
merged 1 commit into from Dec 16, 2019
Merged

feat(list): add the ordered variant #3363

merged 1 commit into from Dec 16, 2019

Conversation

@boaz0
Copy link
Member

boaz0 commented Dec 1, 2019

What:

Closes #3356

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Dec 1, 2019

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

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 1, 2019

Codecov Report

Merging #3363 into master will increase coverage by 0.01%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3363      +/-   ##
==========================================
+ Coverage   67.46%   67.48%   +0.01%     
==========================================
  Files         897      897              
  Lines       25164    25177      +13     
  Branches     2183     2187       +4     
==========================================
+ Hits        16978    16990      +12     
  Misses       7168     7168              
- Partials     1018     1019       +1
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.28% <ø> (ø) ⬆️
#patternfly4 64.93% <94.11%> (+0.02%) ⬆️
Impacted Files Coverage Δ
.../react-core/src/components/LoginPage/LoginPage.tsx 63.63% <100%> (ø) ⬆️
...tternfly-4/react-core/src/components/List/List.tsx 88% <93.75%> (+4.66%) ⬆️

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 9909f88...41e9bc4. Read the comment docs.

@boaz0 boaz0 force-pushed the boaz0:closes_3356 branch from 63b0394 to edd46ad Dec 1, 2019
inline = 'inline'
inline = 'inline',
ordered = 'ordered',
unordered = 'unordered',

This comment has been minimized.

Copy link
@yaacov

yaacov Dec 1, 2019

Contributor

do we need this variant ?

@tlabaj tlabaj added the css review label Dec 2, 2019
@tlabaj tlabaj requested a review from mcoker Dec 2, 2019
@boaz0 boaz0 force-pushed the boaz0:closes_3356 branch from edd46ad to 1256245 Dec 2, 2019
@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Dec 2, 2019

Can variant have more than 1 ListVariant? For example, something like variant={ListVariant.inline, ListVariant.ordered}?

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Dec 2, 2019

@mcoker - I believe we can do that but I wonder why inline should be a variant in that case. Why not using isInline flag to add that modifier?

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Dec 2, 2019

@boaz0 I'm not sure why we didn't use isInline, but ultimately it should be valid to create either an <ol> or <ul> with .pf-m-inline.

Not sure if it helps, but here are some other components that are similar where we allow the user to define the type of element(s) to be used. There are probably more I'm not thinking of.

  • <Button>
    • uses <Button component="a"> to render as an <a> instead of a <button>
  • <Accordion>
    • uses <Accordion asDefinitionList> to render as a <dl> with <dt>'s as titles and <dd>'s as the body, instead of <div>'s with heading elements as the titles.
  • <Tabs>
    • uses <Tabs variant={TabsVariant.nav}> to render as a <nav> instead of a <div>
@boaz0 boaz0 force-pushed the boaz0:closes_3356 branch from 1256245 to 77f902d Dec 3, 2019
@boaz0 boaz0 force-pushed the boaz0:closes_3356 branch 2 times, most recently from b174d70 to 2c697f6 Dec 3, 2019
Copy link
Contributor

tlabaj left a comment

Can you also add a demo for the ordered list in the demo-app and updated the cypress test please.

@boaz0 boaz0 force-pushed the boaz0:closes_3356 branch from 2c697f6 to d6e7f3b Dec 8, 2019
@boaz0 boaz0 force-pushed the boaz0:closes_3356 branch from d6e7f3b to 5dea549 Dec 9, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

Copy link
Contributor

nicolethoen left a comment

@boaz0 I'm not sure why we didn't use isInline, but ultimately it should be valid to create either an <ol> or <ul> with .pf-m-inline.

Was this ever done? I can't seem to make the inline example use <ol> by adding type={OrderType.number}

@boaz0 boaz0 force-pushed the boaz0:closes_3356 branch from 5dea549 to 5f0d0c1 Dec 13, 2019
Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@boaz0 boaz0 force-pushed the boaz0:closes_3356 branch from 5f0d0c1 to 41e9bc4 Dec 13, 2019
@tlabaj
tlabaj approved these changes Dec 13, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

Copy link
Contributor

nicolethoen left a comment

LGTM

@mcoker
mcoker approved these changes Dec 16, 2019
Copy link
Contributor

mcoker left a comment

👍LGTM! Thanks @boaz0!

@tlabaj tlabaj merged commit 373f923 into patternfly:master Dec 16, 2019
9 checks passed
9 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_a11y_pf4 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_3356 branch Dec 17, 2019
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.