feat(dropdown, select): add description element #3130

christiemolloy · 2020-05-22T18:51:06Z

patternfly-build · 2020-05-22T18:53:55Z

Preview: https://patternfly-pr-3130.surge.sh

A11y report: https://patternfly-pr-3130-coverage.surge.sh

mcoker · 2020-05-26T19:37:51Z

src/patternfly/components/AppLauncher/app-launcher-menu-item-description.hbs

@@ -0,0 +1,6 @@
+<p class="pf-c-app-launcher__menu-item-description{{#if app-launcher-menu-item-description--modifier}} {{app-launcher-menu-item-description--modifier}}{{/if}}"
+  {{#if app-launcher-menu-item-description--attribute}}
+	  {{{app-launcher-menu-item-description--attribute}}}


The indentation is off

mcoker · 2020-05-26T19:48:30Z

src/patternfly/components/AppLauncher/app-launcher-menu-item-description.hbs

@@ -0,0 +1,6 @@
+<p class="pf-c-app-launcher__menu-item-description{{#if app-launcher-menu-item-description--modifier}} {{app-launcher-menu-item-description--modifier}}{{/if}}"


Since a <button> only accepts phrasing content, looks like this will need to be a <span>.

mcoker · 2020-05-26T22:32:43Z

src/patternfly/components/AppLauncher/app-launcher.scss

@@ -230,6 +234,10 @@
      --pf-c-app-launcher__menu-item--m-action--Color: var(--pf-c-app-launcher__menu-item--m-action--hover--Color);
    }
  }
+
+  &.pf-m-description {
+    display: block;


Hmm... this is going to cause the styling between non-description and description items to be different. A good example is an item with favorites.

I wonder if this might be a use case for absolutely positioning a pseudo element of __menu-item relative to the parent li so it covers the entire li. Then placing the __menu-item-description element after __menu-item element. So technically they're different elements, but the pseudo element of the __menu-item covers the __menu-item-description element, so clicking on anywhere in the li triggers a click to the __menu-item.

Tried this and it works really well from what i can see!

mattnolting · 2020-06-02T14:39:16Z

src/patternfly/components/AppLauncher/app-launcher.scss

@@ -246,6 +254,11 @@
  }
 }

+.pf-c-app-launcher__menu-item-description {


Looks like this is inheriting white-space: nowrap; Which forces the dropdown to become extra wide.

Can you white-space: normal?

mattnolting · 2020-06-02T14:40:36Z

src/patternfly/components/AppLauncher/examples/AppLauncher.md

+      Menu item disabled
+      {{#> app-launcher-menu-item-description}}
+        This is a description.
+      {{/ app-launcher-menu-item-description}}


Suggested change

{{/ app-launcher-menu-item-description}}

{{/app-launcher-menu-item-description}}

mattnolting · 2020-06-02T14:40:43Z

src/patternfly/components/AppLauncher/examples/AppLauncher.md

+      Menu item default
+      {{#> app-launcher-menu-item-description}}
+        This is a description.
+      {{/ app-launcher-menu-item-description}}


Suggested change

{{/ app-launcher-menu-item-description}}

{{/app-launcher-menu-item-description}}

mattnolting · 2020-06-02T14:40:51Z

src/patternfly/components/AppLauncher/examples/AppLauncher.md

+      Menu item with long description.
+      {{#> app-launcher-menu-item-description}}
+        This is a really long description that describes the menu item. This is a really long description that describes the menu item.
+      {{/ app-launcher-menu-item-description}}


Suggested change

{{/ app-launcher-menu-item-description}}

{{/app-launcher-menu-item-description}}

mattnolting · 2020-06-02T14:42:11Z

src/patternfly/components/AppLauncher/examples/AppLauncher.css

+
+#ws-core-c-applauncher-with-description {
+  min-height: 300px;
+}


Suggested change

}

}

mattnolting · 2020-06-02T14:47:24Z

src/patternfly/components/OptionsMenu/options-menu-menu-item-description.hbs

+    {{{options-menu-menu-item-description--attribute}}}
+  {{/if}}>
+  {{> @partial-block}}
+</p>


Suggested change

</p>

</span>

mattnolting · 2020-06-02T14:48:06Z

src/patternfly/components/Select/select-menu-item-description.hbs

@@ -0,0 +1,6 @@
+<p class="pf-c-select__menu-item-description{{#if select-menu-item-description--modifier}} {{select-menu-item-description--modifier}}{{/if}}"


Suggested change

<p class="pf-c-select__menu-item-description{{#if select-menu-item-description--modifier}} {{select-menu-item-description--modifier}}{{/if}}"

<span class="pf-c-select__menu-item-description{{#if select-menu-item-description--modifier}} {{select-menu-item-description--modifier}}{{/if}}"

mattnolting · 2020-06-02T14:48:16Z

src/patternfly/components/Select/select-menu-item-description.hbs

+    {{{select-menu-item-description--attribute}}}
+  {{/if}}>
+  {{> @partial-block}}
+</p>


Suggested change

</p>

</span>

mattnolting · 2020-06-02T14:54:40Z

src/patternfly/components/OptionsMenu/options-menu.scss

@@ -58,6 +58,11 @@
  --pf-c-options-menu__menu-item--disabled--Color: var(--pf-global--Color--dark-200);
  --pf-c-options-menu__menu-item--hover--BackgroundColor: var(--pf-global--BackgroundColor--light-300);
  --pf-c-options-menu__menu-item--disabled--BackgroundColor: transparent;
+  --pf-c-options-menu__menu-item--m-selected--PaddingRight: var(--pf-global--spacer--2xl);
+  --pf-c-options-menu__menu-item--m-selected__menu-item-icon--Top: 50%;
+  --pf-c-options-menu__menu-item--m-selected__menu-item-icon--Right: 1rem;


Suggested change

--pf-c-options-menu__menu-item--m-selected__menu-item-icon--Right: 1rem;

--pf-c-options-menu__menu-item--m-selected__menu-item-icon--Right: var(--pf-global--spacer--md);

mattnolting · 2020-06-02T14:58:07Z

src/patternfly/components/OptionsMenu/options-menu.scss

+  --pf-c-options-menu__menu-item--m-selected--PaddingRight: var(--pf-global--spacer--2xl);
+  --pf-c-options-menu__menu-item--m-selected__menu-item-icon--Top: 50%;
+  --pf-c-options-menu__menu-item--m-selected__menu-item-icon--Right: 1rem;
+  --pf-c-options-menu__menu-item--m-selected__menu-item-icon--Transform: -50%;


Should this be: --pf-c-options-menu__menu-item--m-selected__menu-item-icon--TranslateY: -50%;?

mcoker · 2020-06-16T18:20:02Z

src/patternfly/components/Dropdown/dropdown-menu-item-description.hbs

@@ -0,0 +1,6 @@
+<span class="pf-c-dropdown__menu-item-description{{#if dropdown-menu-item-description--modifier}} {{dropdown-menu-item-description--modifier}}{{/if}}"


use a <div>

mcoker · 2020-06-18T00:12:31Z

src/patternfly/components/Dropdown/dropdown-menu-item-title.hbs

@@ -0,0 +1,6 @@
+<div class="pf-c-dropdown__menu-item-title{{#if dropdown-menu-item-title--modifier}} {{dropdown-menu-item-title--modifier}}{{/if}}"


I might just call this menu-item-text instead of menu-item-title

Oh sorry I read the code before looking at the examples and assumed it just wrapped the text. Maybe -main?

yeah that sounds good to me

mcoker · 2020-06-18T00:32:52Z

src/patternfly/components/Dropdown/dropdown.scss

+}
+
+.pf-c-dropdown__menu-item-title {
+  display: flex;


Hmm... this is going to make text in the item in a regular dropdown (no description) and an item with a description display differently. I think this flex styling needs to be scoped to when .pf-m-icon is applied to the top level item. Though I see .pf-m-icon is missing from the item in these description examples. Is that intentional?

One way is we could move .pf-m-icon from .pf-c-dropdown__menu-item to .pf-c-dropdown__menu-item-title when a description is used, and update .pf-c-dropdown__menu-item.pf-m-icon { display: flex; align-items: center;} to include .pf-c-dropdown__menu-item-title.pf-m-icon.

You could also do something like this which might be easier on the react side:

li // has icon a.pf-c-dropdown__menu-item.pf-m-description.pf-m-icon // leaves .pf-m-icon on the top level menu-item with or without a description div.pf-c-dropdown__menu-item-title // or whatever you end up calling it span.pf-c-dropdown__menu-item-icon // some icon the dropdown item text div.pf-c-dropdown__menu-item-description the dropdown item description li // no icon a.pf-c-dropdown__menu-item.pf-m-description div.pf-c-dropdown__menu-item-title // or whatever you end up calling it the dropdown item text div.pf-c-dropdown__menu-item-description the dropdown item description

And then the CSS

.pf-c-dropdown__menu-item { &.pf-m-description { flex-direction: column; // fixes the direction when `.pf-m-icon` is applied &.pf-m-icon { .pf-c-dropdown__menu-item-main { display: flex; align-items: center; } } }

mcoker · 2020-06-18T00:35:11Z

src/patternfly/components/Select/select.scss

+}
+
+.pf-c-select__menu-item-description {
+  display: block;


is this necessary?

mcoker · 2020-06-18T00:46:09Z

src/patternfly/components/Select/select.scss

@@ -346,6 +354,14 @@
    right: 0;
  }

+  &.pf-m-wrap {


Not sure if this is the intention but is .pf-m-wrap added to the parent pf-c-select__menu element to allow its children items to wrap? Which allows the description to wrap under the menu-item-title element? If so wouldn't that mean that if .pf-m-wrap is used and some of the items have descriptions and others don't, all of the items will now wrap?

Also, that would require the react component to go back up to the parent __menu to add a class when one of its children meets some condition, which isn't ideal.

What about just adding .pf-m-description to all items with descriptions (not just ones that are selected, as it is currently), and we add white-space: normal; to them, then add .white-space: nowrap; to the __menu-item-title element?

Otherwise this lgtm!

mcoker · 2020-06-18T20:06:49Z

src/patternfly/components/Dropdown/dropdown.scss

+.pf-c-dropdown__menu-item-main {
+  display: flex;
+  align-items: center;
+}


looks like this is still here and doesn't need to be since you added it when it's a child of .pf-c-dropdown__menu-item.pf-m-icon

mattnolting · 2020-06-16T14:53:11Z

src/patternfly/components/Dropdown/examples/Dropdown.md

mattnolting · 2020-06-16T16:48:30Z

src/patternfly/components/Select/select-menu-descriptive.hbs

+      </button>
+    </li>
+    <li>
+      <button type="button" class="pf-c-select__menu-item pf-m-disabled" aria-pressed="false">


should you add disabled here?

mcoker · 2020-06-18T21:02:59Z

src/patternfly/components/Select/select.scss

+  --pf-c-select__menu-item-description--PaddingRight: var(--pf-c-select__menu-item--PaddingRight);
+
+  // Menu item title
+  --pf-c-select__menu-item-main--PaddingRight: var(--pf-global--spacer--2xl);


Should this be...

Suggested change

--pf-c-select__menu-item-main--PaddingRight: var(--pf-global--spacer--2xl);

--pf-c-select__menu-item-main--PaddingRight: var(--pf-c-select__menu-item--m-selected--PaddingRight);

mcoker · 2020-06-18T21:04:08Z

src/patternfly/components/Select/select.scss

@@ -395,6 +403,24 @@
  &.pf-m-selected {
    --pf-c-select__menu-item--PaddingRight: var(--pf-c-select__menu-item--m-selected--PaddingRight);
  }
+
+  &.pf-m-description {
+    --pf-c-select__menu-item--m-selected--PaddingRight: 0;


and if you made the change above, this would change to

Suggested change

--pf-c-select__menu-item--m-selected--PaddingRight: 0;

--pf-c-select__menu-item--PaddingRight: 0;

fix(select, dropdown): updates to description

mcoker

Nice!!

mattnolting

LPTM 🚢 it!

christiemolloy added 6 commits May 20, 2020 17:05

feat(dropdown): add description to menu item

ffdc242

fix(select): add example to select and options menu

4358dce

fix(optionsmenu): make more progress

229ef98

feat(applauncher): add description element

e8435f7

revert files

2904f96

fix(select): update code

c7e962a

christiemolloy requested review from mcoker and mattnolting May 22, 2020 18:51

fix(applauncher): add update

ae6f4ee

mcoker reviewed May 26, 2020

View reviewed changes

mattnolting reviewed Jun 2, 2020

View reviewed changes

christiemolloy added 5 commits June 4, 2020 16:59

fix(description): change p to span

bda5908

fix(applauncher): add pseudo element

02ad293

fix(applauncher): update style

3df258f

fix(applauncher): fix structure

07b7acd

fix(applauncher): update

4b8e5ff

christiemolloy changed the title ~~feat(dropdown, select, applauncher, optionsmenu): add description element~~ feat(dropdown, select): add description element Jun 9, 2020

christiemolloy added 3 commits June 10, 2020 09:30

fix(dropdown): updates

9a88d3b

fix(applauncher): revert changes to app launcher options menu

b3a1d1b

fix(select): add documentation

84f0c55

mcoker reviewed Jun 16, 2020

View reviewed changes

christiemolloy added 3 commits June 16, 2020 16:06

fix(dropdown): updates to dropdown

7aa9527

fix(select): update

b25a494

fix(dropdown): update docs

57a5e59

mcoker reviewed Jun 18, 2020

View reviewed changes

christiemolloy added 2 commits June 18, 2020 10:37

fix(select): update

b1cbc6a

fix(dropdown): make updates

a1d19cf

mcoker reviewed Jun 18, 2020

View reviewed changes

mattnolting reviewed Jun 18, 2020

View reviewed changes

fix(select): fix coker and nolting feedback

55f4b70

mcoker reviewed Jun 18, 2020

View reviewed changes

mcoker and others added 2 commits June 18, 2020 16:13

fix(select, dropdown): updates to description

f601074

Merge pull request #28 from mcoker/select-dropdown-desc

673dc21

fix(select, dropdown): updates to description

mcoker approved these changes Jun 18, 2020

View reviewed changes

mattnolting self-requested a review June 18, 2020 21:27

mattnolting approved these changes Jun 18, 2020

View reviewed changes

mattnolting merged commit ddb4ff3 into patternfly:master Jun 18, 2020

This was referenced Jun 22, 2020

Dropdown - add support for item description patternfly/patternfly-react#4429

Closed

Select - add support for item description patternfly/patternfly-react#4430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dropdown, select): add description element #3130

feat(dropdown, select): add description element #3130

christiemolloy commented May 22, 2020

patternfly-build commented May 22, 2020 •

edited

Loading

mcoker May 26, 2020

mcoker May 26, 2020

mcoker May 26, 2020

christiemolloy Jun 5, 2020

mattnolting Jun 2, 2020

christiemolloy Jun 5, 2020

mattnolting Jun 2, 2020

mattnolting Jun 2, 2020

mattnolting Jun 2, 2020

mattnolting Jun 2, 2020

mattnolting Jun 2, 2020

mattnolting Jun 2, 2020

mattnolting Jun 2, 2020

mattnolting Jun 2, 2020

mattnolting Jun 2, 2020

mcoker Jun 16, 2020

mcoker Jun 18, 2020

mcoker Jun 18, 2020

christiemolloy Jun 18, 2020

mcoker Jun 18, 2020

mcoker Jun 18, 2020

mcoker Jun 18, 2020

mcoker Jun 18, 2020

mattnolting Jun 16, 2020

mattnolting Jun 16, 2020

mcoker Jun 18, 2020

mcoker Jun 18, 2020

mcoker left a comment

mattnolting left a comment

		@@ -0,0 +1,6 @@
		<p class="pf-c-app-launcher__menu-item-description{{#if app-launcher-menu-item-description--modifier}} {{app-launcher-menu-item-description--modifier}}{{/if}}"

	{{/ app-launcher-menu-item-description}}
	{{/app-launcher-menu-item-description}}

		@@ -0,0 +1,6 @@
		<p class="pf-c-select__menu-item-description{{#if select-menu-item-description--modifier}} {{select-menu-item-description--modifier}}{{/if}}"

	<p class="pf-c-select__menu-item-description{{#if select-menu-item-description--modifier}} {{select-menu-item-description--modifier}}{{/if}}"
	<span class="pf-c-select__menu-item-description{{#if select-menu-item-description--modifier}} {{select-menu-item-description--modifier}}{{/if}}"

	--pf-c-options-menu__menu-item--m-selected__menu-item-icon--Right: 1rem;
	--pf-c-options-menu__menu-item--m-selected__menu-item-icon--Right: var(--pf-global--spacer--md);

		@@ -0,0 +1,6 @@
		<span class="pf-c-dropdown__menu-item-description{{#if dropdown-menu-item-description--modifier}} {{dropdown-menu-item-description--modifier}}{{/if}}"

		@@ -0,0 +1,6 @@
		<div class="pf-c-dropdown__menu-item-title{{#if dropdown-menu-item-title--modifier}} {{dropdown-menu-item-title--modifier}}{{/if}}"

	\| `.pf-m-active` \| `.pf-c-dropdown__toggle` \| Forces display of the active state of the toggle. \|
	\| `.pf-m-active` \| `.pf-c-dropdown__toggle` \| Modifies the dropdown menu toggle for the active state. \|

	--pf-c-select__menu-item-main--PaddingRight: var(--pf-global--spacer--2xl);
	--pf-c-select__menu-item-main--PaddingRight: var(--pf-c-select__menu-item--m-selected--PaddingRight);

	--pf-c-select__menu-item--m-selected--PaddingRight: 0;
	--pf-c-select__menu-item--PaddingRight: 0;

feat(dropdown, select): add description element #3130

feat(dropdown, select): add description element #3130

Conversation

christiemolloy commented May 22, 2020

patternfly-build commented May 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

mattnolting left a comment

Choose a reason for hiding this comment

patternfly-build commented May 22, 2020 •

edited

Loading