Skip to content

Adds BorderedMenuItem and use it for delete menu#4359

Merged
benjiwheeler merged 2 commits intoscratchfoundation:developfrom
apple502j:delete-select
Jan 16, 2019
Merged

Adds BorderedMenuItem and use it for delete menu#4359
benjiwheeler merged 2 commits intoscratchfoundation:developfrom
apple502j:delete-select

Conversation

@apple502j
Copy link
Contributor

Resolves

Resolves #4345

Proposed Changes

  • Adds BorderedMenuItem, which has a border at the top and becomes red when hovered
  • Use it for Delete menu (also reorder it)

Red for delete

Reason for Changes

Some people accidentally delete sprites because there was no difference between this and the others

Test Coverage

  • Lint passed
  • Checked on browser

Browser Coverage

Check the OS/browser combinations tested (At least 2)

Mac

  • Chrome
  • Firefox
  • Safari

Windows

  • Chrome
  • Firefox
  • Edge

Chromebook

  • Chrome

iPad

  • Safari

Android Tablet

  • Chrome

@thisandagain
Copy link
Contributor

/cc @carljbowman

@thisandagain thisandagain added this to the January 2019 milestone Jan 16, 2019
Copy link
Contributor

@benjiwheeler benjiwheeler left a comment

Choose a reason for hiding this comment

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

LGTM! I tweaked the css slightly to simplify it, and softened the separator's appearance a tiny bit.

@benjiwheeler benjiwheeler merged commit 2e017f0 into scratchfoundation:develop Jan 16, 2019
@carljbowman
Copy link
Contributor

Sorry I was not able to respond quick enough yesterday, before this was merged.

@apple502j - Thanks for working on this. Great to have a divider style. We def need one in out Context Menu component. Also nice to shift the menu items specifically for this menu.

The separator is a little chunky and feels slightly inconsistent with the rest of the UI. It'd be great to reduce the border down to 1px and make it a transparent black, we should have one in GUI, possibly a 15% or 25%. @benjiwheeler - Do you mind making those changes?

We can keep the red hover for now, but it is something that we should continue to monitor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants