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

NTP Widget Settings design updates #6015

Merged
merged 1 commit into from
Jul 14, 2020
Merged

NTP Widget Settings design updates #6015

merged 1 commit into from
Jul 14, 2020

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Jul 7, 2020

Resolves brave/brave-browser#10318

Note: The removal of "Show" text has been moved to a separate issue/PR brave/brave-browser#10591

Screen Shot 2020-07-06 at 9 41 37 PM

Screen Shot 2020-07-06 at 8 30 19 PM

Submitter Checklist:

Test Plan:

Additional specifications defined in issue for style updates. Functional testing plan with new "Add Card tab"

  1. Build Brave with a clean profile
  2. Navigate to brave://newTab
  3. Hover over the "Add Card" widget, ensure the ellipsis menu is not available.
  4. Remove the Binance and Rewards widget from the stack
  5. Hover over the "Add Card" widget, ensure the menu is available, hide the card.
  6. Under More Cards in the NTP settings, add back either widget.
  7. Ensure the "Add Card" widget appears back, at the very top.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM

@ryanml ryanml force-pushed the feature-10318 branch 2 times, most recently from 17658f0 to 5131b6d Compare July 10, 2020 19:59
@ryanml
Copy link
Contributor Author

ryanml commented Jul 10, 2020

@petemill this one has been rebased

@ryanml ryanml force-pushed the feature-10318 branch 2 times, most recently from 3f53eb0 to 23d7cfa Compare July 11, 2020 08:13
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

The "Add Card" button, when it's on its own looks too tall. That's underlined by the fact that the hover seems to only introduce a shadow covering the part of it which doesn't seem too tall:
image
image

It would be great to at least do a follow-up where we add style transitions. Some of the affects are a bit jarring. Specifically for this PR:

  • "Add Card" shadow on hover getting a transition

but related:

  • Settings popup getting some kind of bounce transition
  • Settings popup growing / shrinking in height when changing tabs (or getting a fixed / percentage-of-viewport height)
  • Widget order should transition / animate when changing

I also think we need a follow-up to address the context menu in that when opening it then mouseout-ing the widget, the menu disappears but re-appears when mouseenter-ing the widget subsequently.

These follow-ups could at least be added to the NTP board for when things calm down a bit on the feature front.

@ryanml
Copy link
Contributor Author

ryanml commented Jul 14, 2020

Screen Shot 2020-07-14 at 1 00 01 AM

Height reduced when the AddCard tab is left alone

Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

Great feature. Thanks for addressing feedback. Found a couple more issues but I don't think they're related as they seem the same on nightly. If not could you add follow-ups?

  1. Is it intentional that both the Add Card and the foreground Card have shadows on hover, but background Cards do not?

i.e. Rewards in this case:
image

  1. Seems like the issue where you can't go from the Card's ... button to the actual context menu is back - I thought we fixed that?

@ryanml
Copy link
Contributor Author

ryanml commented Jul 14, 2020

Failures unrelated

@ryanml ryanml merged commit 6e1c2c7 into master Jul 14, 2020
@ryanml ryanml deleted the feature-10318 branch July 14, 2020 17:39
@rebron
Copy link
Collaborator

rebron commented Aug 5, 2020

@ryanml Milestone for this is 1.13.x right?

@rebron rebron modified the milestones: 1.12.x - Release, 1.13.x - Beta Aug 5, 2020
@karenkliu
Copy link

  1. Is it intentional that both the Add Card and the foreground Card have shadows on hover, but background Cards do not?

It's supposed to be no shadows on hover for Add Card or background cards, but it looks kind of weird with the ghost styling so lets get rid of shadow on hover for all of them.

@ryanml
Copy link
Contributor Author

ryanml commented Aug 5, 2020

Sounds good @karenkliu

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.

[Desktop] design updates to customize dialog follow up to #9455
5 participants