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(description-list): added description list demo #4715

Merged
merged 4 commits into from Mar 2, 2022

Conversation

mattnolting
Copy link
Contributor

closes #1921

@mcarrano Inline edit isn't yet available via React outside of pf-c-table, so I think it's best to add those demo features when it's available globally.

@patternfly-build
Copy link

patternfly-build commented Mar 2, 2022

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

This is looking good @mattnolting . I agree regarding the edit in-place example. I do have a couple of question for @mceledonia , however.

1- When a description list is placed in a page as in these demos, should it be on a white or gray background? The designs show white, however to get that, would we place this inside a card?

2- The drawer example feels a bit dense to me vs the designs, but I can't say why. Anything different that you would recommend @mceledonia ?

Here is the design link: https://marvelapp.com/prototype/6e72b59/screen/85219622

@mcarrano mcarrano requested a review from mceledonia March 2, 2022 14:12
@mattnolting mattnolting force-pushed the feat-description-list-1921 branch 2 times, most recently from c2bd496 to 5618aa7 Compare March 2, 2022 14:29
@mattnolting
Copy link
Contributor Author

1- When a description list is placed in a page as in these demos, should it be on a white or gray background? The designs show white, however to get that, would we place this inside a card?

@mcarrano I can place the description list in a page main section with a white background or a card, but because of the age of the mockups, I assumed the page main section should be white. Happy to update however you want.

2- The drawer example feels a bit dense to me vs the designs, but I can't say why. Anything different that you would recommend @mceledonia ?

I updated the width of the drawer panel to have a bit more room.

@mceledonia
Copy link

mceledonia commented Mar 2, 2022

1- When a description list is placed in a page as in these demos, should it be on a white or gray background? The designs show white, however to get that, would we place this inside a card?

We will want to be sure they work on both, but I do think inside of a card or on a white background is where this will be used most.

2- The drawer example feels a bit dense to me vs the designs, but I can't say why. Anything different that you would recommend @mceledonia ?

I updated the width of the drawer panel to have a bit more room.

This new spacing in the drawer panel looks good to me!

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

LPTM!! Do you need to set the table bg color? Doesn't seem like it's necessary since it's against a white background.

{{/description-list-text}}
{{/description-list-term}}
{{#> description-list-description}}
{{#> table table--id="service-port" table--grid="true" table--modifier="pf-m-grid-md pf-m-compact" table--attribute='aria-label="Service address" style="--pf-c-table--BackgroundColor: transparent"'}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{#> table table--id="service-port" table--grid="true" table--modifier="pf-m-grid-md pf-m-compact" table--attribute='aria-label="Service address" style="--pf-c-table--BackgroundColor: transparent"'}}
{{#> table table--id="service-port" table--grid="true" table--modifier="pf-m-grid-md pf-m-compact" table--attribute='aria-label="Service address"'}}

{{/description-list-text}}
{{/description-list-term}}
{{#> description-list-description}}
{{#> table table--id="service-address" table--grid="true" table--modifier="pf-m-grid-md pf-m-compact" table--attribute='aria-label="Service address" style="--pf-c-table--BackgroundColor: transparent"'}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{#> table table--id="service-address" table--grid="true" table--modifier="pf-m-grid-md pf-m-compact" table--attribute='aria-label="Service address" style="--pf-c-table--BackgroundColor: transparent"'}}
{{#> table table--id="service-address" table--grid="true" table--modifier="pf-m-grid-md pf-m-compact" table--attribute='aria-label="Service address"'}}

@mcoker mcoker merged commit d806ec6 into patternfly:main Mar 2, 2022
@patternfly-build
Copy link

🎉 This PR is included in version 4.181.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Description list demo
5 participants