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.

Sign up for GitHub

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
+48 −102
Open

refactor: wp-791 dropdown menu #1028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ff8cc9e
refactor: WP-791 dropdown button menu
wesleyboar Dec 3, 2024
bb73411
refactor: WP-791 DataFilesBread… dropdown menu
wesleyboar Dec 3, 2024
1b644ed
chore: remove class data-files-btn
wesleyboar Dec 3, 2024
a678c6f
doc: save `refactor/WP-791-dropdown-menu` PR desc
wesleyboar Dec 3, 2024
d6f820b
Revert "chore: remove class data-files-btn"
wesleyboar Dec 3, 2024
2840e42
style: npm run prettier:fix
wesleyboar Dec 4, 2024
1d0317b
chore: remove unnecessary `!important`
wesleyboar Dec 4, 2024
5a54861
enhance: use `<DropdownItem divider>` consistently
wesleyboar Dec 4, 2024
958069b
chore: refactor-WP-791-dropdown-menu-pr-desc.md
wesleyboar Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 7 additions & 24 deletions client/src/components/DataFiles/DataFilesBreadcrumbs/DataFilesBreadcrumbs.scss
View file
Open in desktop
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
7 changes: 3 additions & 4 deletions client/src/components/DataFiles/DataFilesDropdown/DataFilesDropdown.jsx
View file
Open in desktop
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
11 changes: 6 additions & 5 deletions client/src/components/DataFiles/DataFilesSidebar/DataFilesSidebar.jsx
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion client/src/components/DataFiles/DataFilesSidebar/DataFilesSidebar.module.scss
View file
Open in desktop
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. ↩

}
57 changes: 8 additions & 49 deletions client/src/components/DataFiles/DataFilesSidebar/DataFilesSidebar.scss
View file
Open in desktop
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
View file
Open in desktop
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);
}
}