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(PF4 Option): Rewrite options menu to use dropdown component #2299

Merged
merged 2 commits into from Aug 26, 2019

Conversation

@karelhala
Copy link
Contributor

karelhala commented Jun 19, 2019

What:
Changes Options menu to use dropdown components.

  • Fixes problem with navigation (now possible to navigate by arrows)
  • When menu is visible clicking outside closes it
  • Custom icon and text classes for dropdown items

Additional issues:

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 19, 2019

@karelhala karelhala marked this pull request as ready for review Jun 20, 2019
@karelhala karelhala force-pushed the karelhala:options-dropdown branch from a992c83 to 67c1afd Jun 20, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 20, 2019

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

@karelhala karelhala force-pushed the karelhala:options-dropdown branch from 67c1afd to 232b76a Jun 20, 2019
@redallen

This comment has been minimized.

Copy link
Contributor

redallen commented Jun 24, 2019

Dropdown has been touched and has conflicts @karelhala

@karelhala karelhala force-pushed the karelhala:options-dropdown branch from 232b76a to c436892 Jun 25, 2019
Copy link
Contributor

redallen left a comment

Fix merge conflicts and this looks good.

@@ -89,8 +89,13 @@ export class DropdownWithContext extends React.Component {
}
return (
<DropdownContext.Consumer>
<<<<<<< HEAD

This comment has been minimized.

Copy link
@redallen

redallen Jun 25, 2019

Contributor

We don't put heads on pikes anymore.

This comment has been minimized.

Copy link
@karelhala

karelhala Jun 25, 2019

Author Contributor

Lol, good point.

@karelhala karelhala force-pushed the karelhala:options-dropdown branch 2 times, most recently from 714196d to d19688c Jun 25, 2019
@karelhala karelhala requested a review from redallen Jun 26, 2019
Copy link
Contributor

redallen left a comment

The only regression I see is that the pf-m-disabled class and aria-disabled="true" is not added to the disabled <button> anymore in the multiple options example. Good addition of role= and tabindex=-1 to make our Dropdown components more consistent.

<DropdownContext.Consumer>
{({ id: contextId }) => (
<DropdownToggle
{...isPlain || hideCaret ? {

This comment has been minimized.

Copy link
@redallen

redallen Jun 27, 2019

Contributor

Cleaner to do {...(isPlain || hideCaret) && { iconComponent: null }} for the conditional spread next time.

This comment has been minimized.

Copy link
@karelhala

karelhala Jul 1, 2019

Author Contributor

Yeah, that would be easier. Good point

{menuItems}
</ul>}
</div>
<DropdownContext.Provider value={{

This comment has been minimized.

Copy link
@redallen

redallen Jun 27, 2019

Contributor

Yes, thank you!

@dlabaj dlabaj removed their assignment Jun 27, 2019
@karelhala karelhala force-pushed the karelhala:options-dropdown branch 3 times, most recently from 7b4c859 to fbdd17b Jul 1, 2019
Copy link
Contributor

redallen left a comment

Thanks for this refactor!

@karelhala karelhala force-pushed the karelhala:options-dropdown branch from fbdd17b to 960fed8 Jul 3, 2019
@karelhala karelhala requested a review from redallen Jul 3, 2019
@karelhala karelhala force-pushed the karelhala:options-dropdown branch 2 times, most recently from 025b4c6 to 5f1fe9f Jul 3, 2019
Copy link
Contributor

redallen left a comment

Classes are correctly added to the disabled button and functionality is still intact. Thanks!

@karelhala karelhala force-pushed the karelhala:options-dropdown branch 2 times, most recently from 23644e5 to 7e9b86e Jul 12, 2019
@karelhala karelhala force-pushed the karelhala:options-dropdown branch from 7e9b86e to de514ea Jul 17, 2019
@karelhala karelhala force-pushed the karelhala:options-dropdown branch 5 times, most recently from ef6c380 to 6c2dfca Jul 31, 2019
Copy link
Contributor

redallen left a comment

Didn't get a chance to verify all the HTML, but functionality and visuals match up to what we currently have. Nicely done 🎖

{({ menuClass, menuComponent }) => {
const MenuComponent = (menuComponent || Component) as any;
return (
<MenuComponent

This comment has been minimized.

Copy link
@redallen

redallen Aug 6, 2019

Contributor

Could abstract this out to prevent some code duplication.

{children && <span className={IconComponent && css(styles.dropdownToggleText)}>{children}</span>}
{IconComponent && <IconComponent className={css(children && styles.dropdownToggleIcon)} />}
</Toggle>
<DropdownContext.Consumer>

This comment has been minimized.

Copy link
@redallen

redallen Aug 6, 2019

Contributor

MY hero.

Copy link
Contributor

mcoker left a comment

In the disabled variation, not sure if this is reated to #2666, but the pf-c-options-menu__toggle element doesn't need .pf-m-disabled, since it's a <button> element. It only needs the disabled attribute.

Also if you apply isDisabled to the "plain with text" variation example, the .pf-c-options-menu__toggle-button element should have the disabled attribute.

Otherwise looks great!

@karelhala

This comment has been minimized.

Copy link
Contributor Author

karelhala commented Aug 16, 2019

@mcoker thanks for the visual update! I am in no control of which classes are applied to the toggle button sadly. So we have to leave that to #2666. However good point with adding disabled to button of toggle with text. It should be update (I'll also add example of toggle with text that is disabled)

@karelhala karelhala dismissed stale reviews from redallen and jschuler via b79b57d Aug 16, 2019
@karelhala karelhala force-pushed the karelhala:options-dropdown branch from 6c2dfca to b79b57d Aug 16, 2019
@karelhala karelhala requested review from mcoker, jschuler and redallen Aug 16, 2019
@karelhala karelhala force-pushed the karelhala:options-dropdown branch from b79b57d to 6ae31fe Aug 16, 2019
Copy link
Contributor

redallen left a comment

Still looking good!

@mcoker
mcoker approved these changes Aug 22, 2019
Copy link
Contributor

mcoker left a comment

🤠👍

@jschuler jschuler merged commit e18b407 into patternfly:master Aug 26, 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
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Aug 26, 2019

Your changes have been released in:

  • @patternfly/react-core@3.93.0
  • @patternfly/react-docs@4.10.29
  • @patternfly/react-inline-edit-extension@2.11.4
  • demo-app-ts@2.21.8
  • @patternfly/react-table@2.19.4
  • @patternfly/react-topology@2.8.4
  • @patternfly/react-virtualized-extension@1.1.135

Thanks for your contribution! 🎉

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