Skip to content
This repository has been archived by the owner on Aug 1, 2022. It is now read-only.

Dropdown menu component #195

Merged
merged 10 commits into from Mar 4, 2020
Merged

Dropdown menu component #195

merged 10 commits into from Mar 4, 2020

Conversation

sarahscott
Copy link
Contributor

@sarahscott sarahscott commented Feb 27, 2020

Fixes #193 & #194

  • Generalize ProjectTopbar (& associated Menu) into Topbar that can be used in other contexts
  • Fix hover highlighting on Menu buttons (icons were turning purple but text was not)
  • Add an AdditionalActionsDropdown to Design System Guide:
    addlactionsbtn

@xla xla added this to In progress in weekly via automation Feb 27, 2020
@xla xla assigned sarahscott and unassigned rudolfs Feb 27, 2020
@rudolfs
Copy link
Member

rudolfs commented Feb 27, 2020

Nice work!! 🔥

add an icon-only button (right now, Button accommodates icon + text buttons but not icons on their own)

I'd just make the whole dropdown component include a with a vanilla html <button> and add all the styling and functionality in there, as I don't see that we'd reuse this at the moment. We can always extract later when we see duplication.

Here's some points on the code itself (couldn't inline my comments as Github seems to have some service outage atm):

  • So far we've been initialising all props with null unless they come with a default value or the prop is a boolean. Is there any reason to break that convention?
export let menuItems = [];
export let items = [];
  • Perhaps we should extract the "loose" pattern matching argument into
const topbarMenuItems = projectId => [
    {
      icon: Icon.Source,
      title: "Source",
      href: path.projectSource(projectId)
      looseActiveStateMatching: true
    },

active={path.active(item.href, $location, item.looseActiveStateMatching)} />

instead of

active={path.active(item.href, $location, i === 0)} />

In ui/Screen/Project.svelte L101-117: why do we need to wrap everything in ? It seems to render just fine without, when I remove it.

ui/Screen/Project.svelte Outdated Show resolved Hide resolved
@sarahscott sarahscott changed the title [wip] generalizes ProjectTopbar => Topbar; starts on additional actions button Generalizes ProjectTopbar => Topbar; starts on additional actions button Feb 27, 2020
@sarahscott sarahscott changed the title Generalizes ProjectTopbar => Topbar; starts on additional actions button [wip] Generalizes ProjectTopbar => Topbar; starts on additional actions button Feb 27, 2020
Copy link
Member

@rudolfs rudolfs left a comment

Choose a reason for hiding this comment

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

Getting close!! 🌟

As for next steps: For the sake of keeping things moving fast, I would suggest we narrow the scope this PR to just making this a generic dropdown menu component and adding it to the design system guide. Then we can follow up with another PR for the specific use-cases (top bar, project and member lists) and the copy-to-clipboard functionality.

ui/DesignSystem/Primitive/Icon/ArrowUp.svelte Outdated Show resolved Hide resolved
ui/DesignSystem/Primitive/Icon/ArrowUp.svelte Outdated Show resolved Hide resolved
ui/DesignSystem/Primitive/Icon/Copy.svelte Show resolved Hide resolved
@sarahscott sarahscott changed the title [wip] Generalizes ProjectTopbar => Topbar; starts on additional actions button Generalizes ProjectTopbar => Topbar; starts on additional actions button Mar 2, 2020
@sarahscott sarahscott changed the title Generalizes ProjectTopbar => Topbar; starts on additional actions button Dropdown menu component Mar 2, 2020
@sarahscott sarahscott added ui feature Something that doesn't exist yet labels Mar 2, 2020
* Generalize ProjectTopbar to Topbar
* Fix hover on MenuItems
* Add additional actions button & modal
Copy link
Member

@rudolfs rudolfs left a comment

Choose a reason for hiding this comment

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

It's coming together! 🎉

Besides the inline comments:

  • can we automatically close the dropdown menu once something is clicked?
  • clicking outside of the modal to close it works only if I click above the menu, if I click on either side or below it doesn't close, is there an easy fix?

ui/DesignSystem/Component/NotificationFaucet.svelte Outdated Show resolved Hide resolved
ui/Screen/DesignSystemGuide.svelte Outdated Show resolved Hide resolved
ui/DesignSystem/Component/AdditionalActionsDropdown.svelte Outdated Show resolved Hide resolved
ui/DesignSystem/Component/AdditionalActionsDropdown.svelte Outdated Show resolved Hide resolved
ui/DesignSystem/Component/AdditionalActionsDropdown.svelte Outdated Show resolved Hide resolved
weekly automation moved this from In progress to In review Mar 2, 2020
Copy link
Contributor

@MeBrei MeBrei left a comment

Choose a reason for hiding this comment

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

Great work!
Added a few small comments.

@@ -27,6 +27,7 @@
--color-lightgray-tint-10: #e9edf1;
--color-lightgray: #ced8e1;
--color-lightgray-shade-10: #b3c3d1;
--color-light-shadow-gray: rgba(51, 51, 51, 0.08);
Copy link
Contributor Author

@sarahscott sarahscott Mar 3, 2020

Choose a reason for hiding this comment

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

@MeBrei @rudolfs does this make sense here or in global.css?

Copy link
Contributor

Choose a reason for hiding this comment

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

color.css will be overwritten once you run yarn generate:colors the next time. I would put it in general.css for now, but maybe write an issue in the Housekeeping milestone to include transparent colors in the color generation.

@xla xla removed the request for review from garbados March 3, 2020 16:11
@@ -27,6 +27,7 @@
--color-lightgray-tint-10: #e9edf1;
--color-lightgray: #ced8e1;
--color-lightgray-shade-10: #b3c3d1;
--color-light-shadow-gray: rgba(51, 51, 51, 0.08);
Copy link
Member

Choose a reason for hiding this comment

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

colors.css is auto-generated and will be overwritten next time someone runs yarn generate:colors.

Let's put this CSS variable in public/global.css for now.
I also think --color-lightgray-transparency-08 would better fit our current naming scheme.

@@ -48,9 +49,11 @@
right: 0;
width: 240px;
margin-top: 15px;
background-color: white;
box-shadow: 0px 4px 8px rgba(51, 51, 51, 0.08);
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@rudolfs rudolfs left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

weekly automation moved this from In review to Approved Mar 3, 2020
@sarahscott sarahscott merged commit c661b21 into master Mar 4, 2020
weekly automation moved this from Approved to Done Mar 4, 2020
@sarahscott sarahscott deleted the sos/topbar branch March 4, 2020 11:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Something that doesn't exist yet
Projects
No open projects
weekly
  
Done
Development

Successfully merging this pull request may close these issues.

Additional Actions Modal
3 participants