Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

Change Background Settings for New Tabs Page #485

Merged
merged 1 commit into from
Jun 5, 2019
Merged

Change Background Settings for New Tabs Page #485

merged 1 commit into from
Jun 5, 2019

Conversation

imptrx
Copy link
Contributor

@imptrx imptrx commented May 30, 2019 •
edited
Loading

Feature Spec: brave/brave-browser#4523

Changes

  • Add a new "Settings" Icon at the foot of the page
  • Clicking the icon will bring up a menu with one toggle option
  • Clicking the toggle will change the background to be an image (with a description at the footer versus a gradient background)
  • Dark mode will bring up a different system menu
    Quick view:
    change-background

Test plan

Go to storybook and make the following assertions:

  1. Ensure menu is toggle-able and exits upon outside click
  2. Ensure background is toggle-able from within the menu option
  3. Ensure responsiveness (super small screens specs TBD)
  4. Ensure app supports dark mode (see knobs)
Link / storybook path to visual changes

http://localhost:9091/?path=/story/feature-components-new-tab-default--page
https://brave-ui.imptrx.now.sh/?path=/story/feature-components-new-tab-default--page

Integration

  • Does this contain changes to src/components or src/

    • Will you publish to npm immediately after this PR, or wait until sometime in the future?
    • Incompatible API change to something existing (major version increase)
    • Adding new backwards-compatible functionality? (minor version increase)
    • Fixing a bug backwards-compatibly? (patch version increase)

  • Does this contain changes to src/features for brave-core?

    • Are there non backwards-compatible changes required for brave-core? Do not merge until brave-core PR is approvable. Link to brave-core PR:
    • Will you create brave-core PR to update to this commit after it is merged?
    • Wants uplift to brave-core feature branch?
      • When uplift-approved, merge to brave-core-0.VV.x feature branch
      • Create additional brave-core PRs for each feature branch to update commit

@imptrx imptrx requested a review from cezaraugusto May 30, 2019 00:26
@imptrx imptrx self-assigned this May 30, 2019
@imptrx
Copy link
Contributor Author

imptrx commented May 30, 2019 •
edited
Loading

The gradient background currently doesn't match designs due to how the display angle positions the starting point color and end point color beyond what's on the actual display. Might need a workaround there

petemill
petemill reviewed May 31, 2019
View reviewed changes
src/theme/theme-interface.ts
@@ -4,6 +4,8 @@ export default interface IThemeProps {
name: string,
palette: { [key: string]: string }
color: {
contextMenuBackground: string
Copy link
Member

Choose a reason for hiding this comment

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

🎆

@bsclifton
Copy link
Member

This is amazing - great work, @imptrx! 😄

@petemill
Copy link
Member

petemill commented Jun 3, 2019

Thanks for addressing feedback @imptrx - please can you squash the commits so that we're only introducing the final code into the repo? This also gives higher chance that the commit messages people see when blaming certain lines are actual features. Thanks again!

@imptrx imptrx force-pushed the toggle-background-img branch 2 times, most recently from 7113929 to d11e35a Compare June 3, 2019 21:11
@karenkliu
Copy link

Hi @imptrx , this works great! The only feedback I have is that the focus indicator is a little too small:

focus indicator

cezaraugusto
cezaraugusto reviewed Jun 4, 2019
View reviewed changes
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

left some comments

src/features/newTab/default/page/index.ts Outdated Show resolved Hide resolved
src/features/newTab/default/settings/index.ts Outdated Show resolved Hide resolved
src/features/newTab/default/settings/index.ts Outdated Show resolved Hide resolved
cezaraugusto
cezaraugusto approved these changes Jun 5, 2019
View reviewed changes
Copy link
Contributor

@cezaraugusto cezaraugusto 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 here!

@cezaraugusto cezaraugusto merged commit dbc5cda into master Jun 5, 2019
@cezaraugusto cezaraugusto deleted the toggle-background-img branch June 5, 2019 13:27
NejcZdovc added a commit that referenced this pull request Jun 5, 2019
@NejcZdovc
Revert "Merge pull request #485 from brave/toggle-background-img"
83c5ddc 
This reverts commit dbc5cda, reversing
changes made to 0139aa3.
NejcZdovc added a commit that referenced this pull request Jun 5, 2019
@NejcZdovc
Merge pull request #490 from brave/revert-485
82b3e98 
Revert "Merge pull request #485 from brave/toggle-background-img"
cezaraugusto added a commit that referenced this pull request Jun 5, 2019
@cezaraugusto
Undo revert of #485
e6a3778 
Revert "Merge pull request #490 from brave/revert-485"

This reverts commit 82b3e98, reversing
changes made to b0ec08c.
@cezaraugusto cezaraugusto mentioned this pull request Jun 5, 2019
Merged
@imptrx imptrx mentioned this pull request Jun 12, 2019
Merged
29 tasks
imptrx pushed a commit that referenced this pull request Jun 13, 2019
@cezaraugusto @imptrx
Undo revert of #485
3a0f21a 
Revert "Merge pull request #490 from brave/revert-485"

This reverts commit 82b3e98, reversing
changes made to b0ec08c.
cezaraugusto added a commit that referenced this pull request Jun 19, 2019
@cezaraugusto
Merge pull request #491 from brave/toggle-background-img-2
92a28b0 
Undo revert of #485
petemill pushed a commit to petemill/brave-core that referenced this pull request Jul 3, 2019
@cezaraugusto @petemill
Undo revert of brave/brave-ui#485
0e4145a 
Revert "Merge pull request brave#490 from brave/revert-485"

This reverts commit 82b3e981040b2ba89743108e6701f5fa24ce3b52, reversing
changes made to b0ec08cd4ed2a20401d058062f1c238fb9a67813.
petemill pushed a commit to petemill/brave-core that referenced this pull request Jul 5, 2019
@cezaraugusto @imptrx
Imported from brave-ui: Undo revert of brave/brave-ui#485
f02ce47 
Revert "Merge pull request brave#490 from brave/revert-485"

This reverts commit 82b3e981040b2ba89743108e6701f5fa24ce3b52, reversing
changes made to b0ec08cd4ed2a20401d058062f1c238fb9a67813.
petemill pushed a commit to petemill/brave-core that referenced this pull request Jul 5, 2019
@cezaraugusto
Imported from brave-ui: Merge pull request brave#491 from brave/toggl…
29831c3 
…e-background-img-2

Undo revert of brave/brave-ui#485
petemill pushed a commit to brave/brave-core that referenced this pull request Jul 5, 2019
@cezaraugusto @petemill
Imported from brave-ui: Undo revert of brave/brave-ui#485
a97cf11 
Revert "Merge pull request #490 from brave/revert-485"

This reverts commit 82b3e981040b2ba89743108e6701f5fa24ce3b52, reversing
changes made to b0ec08cd4ed2a20401d058062f1c238fb9a67813.
@imptrx imptrx mentioned this pull request Aug 7, 2019
Merged
32 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petemill petemill petemill left review comments

@cezaraugusto cezaraugusto cezaraugusto approved these changes

Assignees

@imptrx imptrx

Labels
features/newtab
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@imptrx @bsclifton @petemill @karenkliu @cezaraugusto