convert dropdown markup to pf4 #1629

priley86 · 2019-05-29T20:10:14Z

PR to convert existing dropdowns over to PF4 HTML/CSS. Conversions to PF-React JS would come later...

Actions Detail Dropdown
Note: if the "Add Secret to Workload" button remains unchanged and uses Bootstrap styles, it will not match PF4 Dropdown styles. Should we extend the scope of this to include PF4 Button conversion globally? That should not be terrible, even though we have a lot of BS3 <button>'s still.

Update: An alternative which may work for now and allow us to avoid restyling all Button's now is just updating the co-action-button styles to be height: 100%; . This would limit the scope of this to just Dropdown's.

Without this, we have differently sized Buttons.

Projects Dropdown
This one remains unchanged. It is an exception and does not follow the norm of the other Dropdowns, even though some code is shared. This is a better fit for the PF4 Context Selector i.m.h.o.

Add Dropdown

Kebab Dropdown
This is how it looks with the table as it is today. With the PF4 Table, see this comment

Create Dropdown

Masthead Dropdown
Made fonts 14px globally for menu items to match our interim font size standard. These would be affected too.

cc: @spadgett

openshift-ci-robot · 2019-05-29T20:10:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: priley86
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: rhamilto

If they are not already assigned, you can assign the PR to them by writing /assign @rhamilto in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

frontend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

priley86 · 2019-05-29T20:13:09Z

frontend/public/components/utils/headings.tsx

  {_.map(actionButtons, (actionButton, i) => {
    if (!_.isEmpty(actionButton)) {
-      return <button className={`btn ${actionButton.btnClass} co-action-buttons__btn`} onClick={actionButton.callback} key={i}>{actionButton.label}</button>;
+      return <Button variant="primary" onClick={actionButton.callback} key={i}>{actionButton.label}</Button>;


todo: we can look at PF4 Button variations for this when mapping actionButton.btnClass
http://patternfly-react.surge.sh/patternfly-4/components/button/

openshift-ci-robot · 2019-05-29T20:30:53Z

@priley86: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`458a083`	link	`/test e2e-aws`
ci/prow/e2e-aws-console	`458a083`	link	`/test e2e-aws-console`
ci/prow/e2e-aws-console-olm	`458a083`	link	`/test e2e-aws-console-olm`
ci/prow/images	`458a083`	link	`/test images`
ci/prow/frontend	`458a083`	link	`/test frontend`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

spadgett · 2019-05-30T17:12:46Z

frontend/public/components/namespace.jsx

    ];

-    return <div className="co-namespace-bar__dropdowns">
+    return <div className="co-namespace-bar__dropdowns pf-l-flex pf-m-justify-content-space-between pf-m-align-items-center">


No flexbox utility classes please :)

We went down this road in the 3.x console and eventually removed them all. They have a lot of drawbacks.

This is basically the same as inline styles, which is considered a bad practice.

They don't play well with media queries.

If we ever need a flexbox style that doens't have a utility, it's a lot of work to convert this to CSS.

Let's replace this with proper CSS definitions that follow BEM.

+1 - i can see how this would be problematic if utility classes were reused in several places and then expected to change in only a few. Will refactor this to use custom classes that use BEM for now. PF4 may introduce more specific layout classes for namespace bar in the future.

spadgett · 2019-05-30T17:13:24Z

frontend/public/vendor.scss

+@import '~@patternfly/patternfly/layouts/Flex/flex';
+@import '~@patternfly/patternfly/utilities/Display/display';
+@import '~@patternfly/patternfly/utilities/Flex/flex';
+


Remove extra newlines

spadgett · 2019-05-30T17:16:17Z

frontend/public/components/utils/dropdown.jsx

+    if(!autocompleteFilter){
+      //use pf4 markup if not using the autocomplete dropdown
+      return <li key={itemKey}>
+        <button className="pf-c-dropdown__menu-item" id={`${itemKey}-link`} onClick={e => onclick(itemKey, e)}>{content}</button>


This probably will break enter key usage in most forms. Add type="button" so it's never treated as a submit button. We might need prevent default on the click event.

spadgett · 2019-05-30T17:17:40Z

frontend/public/components/utils/dropdown.jsx

+    const {itemKey, content, onclick, onBookmark, onUnBookmark, className, selected, hover, canFavorite, onFavorite, favoriteKey, autocompleteFilter} = this.props;
    let prefix;
+
+    if(!autocompleteFilter){


style nit, here and below

Suggested change

if(!autocompleteFilter){

if (!autocompleteFilter) {

spadgett · 2019-05-30T17:19:32Z

frontend/public/components/utils/dropdown.jsx

-          <div className="btn-dropdown__content-wrap">
-            <span className="btn-dropdown__item">
+    //render PF4 dropdown markup if this is not the autcomplete filter
+    if(autocompleteFilter){


I'm hesitant to keep two totally separate markup structures for these two dropdowns, but I don't have a great suggestion. Do we have a long term plan for avoiding this?

we will have to see if we can covert the Project/Namespace dropdown to something that resembles PF4. I have not attempted this yet, but I would probably save that one for last if possible...

spadgett · 2019-05-30T17:26:19Z

frontend/public/components/utils/headings.tsx

        {isDeleting && <ResourceItemDeleting />}
      </div>
-      {!isDeleting && <div className="co-actions">
+      {!isDeleting && <div className="pf-l-flex pf-m-space-items-sm">


Remove flex utilities

spadgett · 2019-05-30T17:26:36Z

frontend/public/components/utils/kebab.tsx


 const KebabItemEnabled: React.FC<KebabItemProps> = ({option, onClick}) => {
-  return <a href="#" onClick={(e) => onClick(e, option)} data-test-action={option.label}>{option.label}</a>;
+  return <button className="pf-c-dropdown__menu-item" onClick={(e) => onClick(e, option)} data-test-action={option.label}>{option.label}</button>;


type="button"

spadgett · 2019-05-30T17:26:41Z

frontend/public/components/utils/kebab.tsx


 const KebabItemDisabled: React.FC<{option: KebabOption}> = ({option}) => {
-  return <a className="disabled">{option.label}</a>;
+  return <button className="pf-c-dropdown__menu-item pf-m-disabled">{option.label}</button>;


type="button"

spadgett · 2019-05-30T17:26:56Z

frontend/public/components/utils/kebab.tsx

-    return <div ref={this.dropdownElement} className="co-kebab" onMouseEnter={this.onHover} onFocus={this.onHover}>
-      <button type="button" aria-label="Actions" disabled={isDisabled} aria-haspopup="true" className="btn btn-link co-kebab__button" onClick={this.toggle} data-test-id="kebab-button">
-        <span className="fa fa-ellipsis-v co-kebab__icon" aria-hidden="true"></span>
+    return <div ref={this.dropdownElement} className={classNames({'pf-c-dropdown': true, 'pf-m-expanded': this.state.active})} onFocus={this.onHover}>


Fix classNames

spadgett · 2019-05-30T17:27:11Z

frontend/public/components/utils/kebab.tsx


 KebabItems.displayName = 'KebabItems';
-ResourceKebab.displayName = 'ResourceKebab';
+ResourceKebab.displayName = 'ResourceKebab';


Missing final newline

spadgett · 2019-05-30T17:30:18Z

Create buttons on different pages now look drastically different.

spadgett · 2019-05-30T17:36:50Z

The white button on the white page has very little contrast. I worry about accessibility because the outlines are so subtle. Someone with poor vision might not realize they're buttons.

spadgett · 2019-05-30T17:41:23Z

The different dropdown styles in the same form is jarring.

spadgett · 2019-05-30T17:42:14Z

Another example:

spadgett · 2019-05-30T17:53:37Z

The dropdown font is much larger than the input text. It looks mismatched.

I was hoping to convert form controls together to keep visual consistency. This is one of the reasons I have been proposing working on the dropdowns separate from the table updates.

priley86 · 2019-05-30T17:59:08Z

thanks for the thorough review @spadgett 🙏 - will try to review these items further today.

matthewcarleton · 2019-06-05T19:46:30Z

hey @priley86 @spadgett
I've created an issue to update the dropdown so we can get a primary styling here
that should help with some of the issues mentioned above.

priley86 · 2019-06-07T15:16:21Z

The different dropdown styles in the same form is jarring.

This dropdown can be converted similarly.

priley86 · 2019-06-07T15:18:36Z

Another example:

Same here - this dropdown has very similar markup to the others converted and we can just ensure the custom icon is rendered alongside.

openshift-ci-robot · 2019-06-11T23:33:33Z

@priley86: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

priley86 · 2019-06-19T13:40:49Z

closing in favor of #1751

convert dropdown markup to pf4

458a083

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 29, 2019

openshift-ci-robot requested review from rhamilto and sg00dwin May 29, 2019 20:10

priley86 commented May 29, 2019

View reviewed changes

priley86 mentioned this pull request May 29, 2019

patternfly react-tables #1465

Merged

89 tasks

spadgett added this to the v4.2 milestone May 29, 2019

spadgett requested changes May 30, 2019

View reviewed changes

priley86 mentioned this pull request Jun 7, 2019

Consider Primary modifier for dropdown patternfly/patternfly#1872

Closed

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2019

priley86 closed this Jun 19, 2019

priley86 mentioned this pull request Jun 19, 2019

Convert dropdowns and related inputs to pf4 #1751

Merged

convert dropdown markup to pf4 #1629

convert dropdown markup to pf4 #1629

Conversation

priley86 commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented May 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented May 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spadgett commented May 30, 2019

Uh oh!

spadgett commented May 30, 2019

Uh oh!

spadgett commented May 30, 2019

Uh oh!

spadgett commented May 30, 2019

Uh oh!

priley86 commented May 30, 2019

Uh oh!

matthewcarleton commented Jun 5, 2019

Uh oh!

priley86 commented Jun 7, 2019

Uh oh!

priley86 commented Jun 7, 2019

Uh oh!

openshift-ci-robot commented Jun 11, 2019

Uh oh!

priley86 commented Jun 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

priley86 commented May 29, 2019 •

edited

Loading

priley86 Jun 7, 2019 •

edited

Loading

spadgett commented May 30, 2019 •

edited

Loading