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

refactor: wp-791 dropdown menu #1028

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Copy link
Member Author

Choose a reason for hiding this comment

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

What did I do?

  • Use class not ID.
  • Remove unnecessary styles.
  • Remove styles that dropdown-menu.css manages.
  • Position via top not margin-top.
    Margin is for space between items. Positioning uses specific properties.

Original file line number Diff line number Diff line change
Expand Up @@ -42,32 +42,15 @@
padding-left: var(--horizontal-buffer);
}

/* Nested to prevent styles from affecting CMS header dropdown */
/* HACK: Using ID to increase specificity (until source of problem is fixed) */
/* HELP: Why does DataFilesSidebar not need such specificity? */
/* .go-to-button-dropdown { */
#go-to-button-dropdown {
/* To fix menu not showing */
/* HELP: Why does DataFilesSidebar not need this fix? */
.go-to-button-dropdown {
.dropdown-menu {
opacity: 1 !important;
pointer-events: auto !important;
}
/* To restyle */
.dropdown-menu {
margin-top: 38px;
/* To push menu down by the height of its button */
/* FAQ: Required because `tag={Button}` loses auto-position of menu */
top: 28px !important; /* to override `inset` from Popper via Reactstrap */
}
.dropdown-menu::before,
.dropdown-menu::after {
left: 23px;
margin-left: 0;
}
.dropdown-menu::after {
top: -9px;
}

.dropdown-item {
display: inline-block;
}
}

Expand All @@ -85,12 +68,12 @@
padding-left: 20px;
}

#go-to-button-dropdown .complex-dropdown-item-root,
.complex-dropdown-item-root,
.complex-dropdown-item-project {
display: flex !important;
display: flex;
}
Comment on lines -88 to 74
Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

  • Classes are custom and unique, so no need to scope them.1
  • The style works without !important.

Footnotes

  1. Ideally, the classes are modular from DataFilesBreadcrumbs.module.css but I'm trying to keep this diff light.


#go-to-button-dropdown .link-hover:hover {
.go-to-button-dropdown .link-hover:hover {
text-decoration: none;
}

Expand Down
wesleyboar marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

  • Latest Reactstrap does not document ButtonDropdown; it might be deprecated.

Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ import PropTypes from 'prop-types';
import { Link, useLocation } from 'react-router-dom';
import { Button } from '_common';
import {
Dropdown,
DropdownToggle,
DropdownMenu,
DropdownItem,
ButtonDropdown,
} from 'reactstrap';
import { useSystemDisplayName, useSystems } from 'hooks/datafiles';
import '../DataFilesBreadcrumbs/DataFilesBreadcrumbs.scss';
Expand Down Expand Up @@ -75,10 +75,9 @@ const BreadcrumbsDropdown = ({
const sliceStart = scheme === 'projects' && systemName ? 0 : 1;
return (
<div id="path-button-wrapper">
<ButtonDropdown
<Dropdown
isOpen={dropdownOpen}
toggle={toggleDropdown}
id="go-to-button-dropdown"
className="go-to-button-dropdown"
>
<DropdownToggle tag={Button}>Go to ...</DropdownToggle>
Expand Down Expand Up @@ -132,7 +131,7 @@ const BreadcrumbsDropdown = ({
</DropdownItem>
</Link>
</DropdownMenu>
</ButtonDropdown>
</Dropdown>
</div>
);
};
Expand Down
wesleyboar marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

  • Don't use wrappers (cruft) unless necessary.
  • Latest Reactstrap does not document ButtonDropdown; it might be deprecated.
  • Use <DropdownItem divider /> like DataFilesDropdown does.
    If we're gonna use Reactstrap, let's use it as designed (until we replace it).1

Footnotes

  1. A more recent guideline I follow, cuz it's easier to reference an API than search for "what we normally do".

Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
Nav,
NavItem,
NavLink,
ButtonDropdown,
Dropdown,
DropdownMenu,
DropdownToggle,
DropdownItem,
Expand Down Expand Up @@ -67,8 +67,8 @@ const DataFilesAddButton = ({ readOnly }) => {
const writeItemStyle = disabled ? 'read-only' : '';

return (
<div id="add-button-wrapper">
<ButtonDropdown isOpen={dropdownOpen} toggle={toggleDropdown}>
<>
<Dropdown isOpen={dropdownOpen} toggle={toggleDropdown}>
<DropdownToggle
color="primary"
id="data-files-add"
Expand All @@ -89,6 +89,7 @@ const DataFilesAddButton = ({ readOnly }) => {
<i className="icon-folder" /> Shared Workspace
</DropdownItem>
)}
<DropdownItem divider />
<DropdownItem
className={`complex-dropdown-item ${styles[writeItemStyle]}`}
onClick={toggleUploadModal}
Expand All @@ -101,8 +102,8 @@ const DataFilesAddButton = ({ readOnly }) => {
</span>
</DropdownItem>
</DropdownMenu>
</ButtonDropdown>
</div>
</Dropdown>
</>
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@

.add-button-item {
padding-bottom: 20px;
padding-left: 20px;
padding-inline: var(--global-space--section-left);
Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

  • To center the (full-width) button, instead of just push it to the left.1

Footnotes

  1. I would also use only margin not padding, but both work here and I try to keep diff light.

}
Copy link
Member Author

Choose a reason for hiding this comment

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

Why delete so much?
Most of the dropdown menu styles are in dropdown-menu.css thus are redundant here.

Original file line number Diff line number Diff line change
Expand Up @@ -10,70 +10,29 @@
border-radius: 0;
}

#add-button-wrapper {
padding-left: var(--horizontal-buffer);
}
#data-files-add {
width: 174px;
width: 100%;
}
Comment on lines -13 to 15
Copy link
Member Author

Choose a reason for hiding this comment

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

What did I do? Match design:

  • Stretch the button.
  • Center the button.


.data-files-nav {
padding-top: 20px;
}

/* Nested to prevent styles from affecting CMS header dropdown */
.data-files-sidebar {
.dropdown-menu {
border-color: var(--global-color-accent--normal);
border-radius: 0;
margin: 11px 3px 0;
width: 200px;
vertical-align: top;
}
.dropdown-menu::before {
position: absolute;
top: -10px;
left: 67px;
border-right: 10px solid transparent;
border-bottom: 10px solid var(--global-color-accent--normal);
border-left: 10px solid transparent;
margin-left: 20px;
content: '';
/* To make menu wider than button yet still centered */
width: calc(100% + var(--global-space--section-left));
left: calc(
var(--global-space--section-left) / -2
) !important; /* to override `inset` from Popper via Reactstrap */
}
.dropdown-menu::before,
.dropdown-menu::after {
top: -9px;
left: 68px;
border-right: 9px solid transparent;
border-bottom: 9px solid #ffffff;
border-left: 9px solid transparent;
margin-left: 20px;
content: '';
}

.dropdown-item {
padding: 10px 6px;
color: var(--global-color-primary--x-dark);
font-size: 14px;
i {
padding-right: 19px;
font-size: 20px;
vertical-align: middle;
}
&:hover {
color: var(--global-color-primary--x-dark);
}
}
.dropdown-item:focus,
.dropdown-item:hover {
/* FAQ: Before FP-1083, value was #E6E0FB, which matched Design
and was `--global-color-accent` at 25% opacity on white…
which is what `--global-color-accent--weak` is now */
background-color: var(--global-color-accent--weak);
left: calc(50% - var(--arrow-size));
}
}

.complex-dropdown-item {
border-top: 1px solid #707070;
Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

  • Replaced by <DropdownItem divider>.

display: flex;
}

Expand Down
42 changes: 23 additions & 19 deletions client/src/styles/components/dropdown-menu.css
wesleyboar marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member Author

Choose a reason for hiding this comment

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

What did I do?

  • Add variables so arrow math is documented via code.
  • Style <DropdownItem divider>.
  • Clarify code with whitespace or comments.

Original file line number Diff line number Diff line change
Expand Up @@ -6,32 +6,34 @@ A menu of navigation elements.
Styleguide Components.Dropdown
*/

/* Nested to prevent styles from affecting CMS header dropdown */
.workbench-wrapper {
.workbench-wrapper /* nested so CMS header dropdown is unaffected */ {
.dropdown-menu {
border-color: var(--global-color-accent--normal);
border-radius: 0;
margin-top: 11px;
padding: 0;
min-width: 200px;
width: auto;
vertical-align: top;
--border-width: 1px;
--border-color: var(--global-color-accent--normal);
--arrow-size: 10px;

margin-top: var(--arrow-size); /* to make space for arrow */

padding-block: unset; /* to undo Bootstrap */
border-radius: unset; /* to undo Bootstrap */

border: var(--border-width) solid var(--border-color);
}
.dropdown-menu::before,
.dropdown-menu::after {
position: absolute;
left: 65px;
border-right: 10px solid transparent;
border-bottom: 10px solid var(--global-color-accent--normal);
border-left: 10px solid transparent;
margin-left: 20px;

border-right: var(--arrow-size) solid transparent;
border-bottom: var(--arrow-size) solid var(--global-color-accent--normal);
border-left: var(--arrow-size) solid transparent;

content: '';
}
.dropdown-menu::before {
top: -10px;
top: calc(var(--arrow-size) * -1);
}
.dropdown-menu::after {
top: -9px;
top: calc(( var(--arrow-size) - var(--border-width)) * -1);
border-bottom-color: var(--global-color-primary--xx-light);
}

Expand All @@ -50,9 +52,11 @@ Styleguide Components.Dropdown
}
.dropdown-item:focus,
.dropdown-item:hover {
/* FAQ: Before FP-1083, value was #E6E0FB, which matched Design
and was `--global-color-accent` at 25% opacity on white…
which is what `--global-color-accent--weak` is now */
background-color: var(--global-color-accent--weak);
}

.dropdown-divider {
margin-block: 0.25em;
border-color: var(--global-color-primary--dark);
}
}
50 changes: 50 additions & 0 deletions refactor-WP-791-dropdown-menu-pr-desc.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
## Overview

Refactor the styles for Data Files dropdown menus to:
- share more CSS
- use clearer CSS

## Related

* [WP-791](https://tacc-main.atlassian.net/browse/WP-791)
* inspired by https://github.com/TACC/Core-Portal/pull/1018#pullrequestreview-2476250609

## Changes

**`.dropdown-menu`**:
- remove redundant code
- use `<Dropdown>` not `<ButtonDropdown>`[^1]
- clean up styles
- use CSS var's for arrow and border

**`DataFilesBreadcrumbs`**:
- fix layout of menu and button

**`DataFilesBreadcrumbs`**:
- use class not ID
- position via `top` not `margin-top`
- explain menu position value
- explain why positioning is necessary[^2]

[^1]: I don't see `ButtonDropdown` in [latest ReactStrap (v9)](https://reactstrap.github.io/?path=/docs/components-dropdown--dropdown).
[^2]: Not desired, but necessary if we use our custom `<Button>` as is.

## Testing

1. Open Data Files.
2. Click "Add +" button.
- Verify button is centered.
- Verify menu is centered.
- Verify menu is wider than button.
- Verify elements are otherwise unchanged.
4. Click "Go to ..." button.
- Verify elements are unchanged.

## UI

| `DataFilesDropdown` | `DataFilesSidebar` |
| - | - |
| <img width="400" alt="DataFilesDropdown" src="https://github.com/user-attachments/assets/c0a35d5b-d304-4625-b94e-6ab7888ec3e4"> | <img width="400" alt="DataFilesSidebar" src="https://github.com/user-attachments/assets/7bec0d60-ec77-45d7-b53d-7c0688d8c75b"> |

## Notes