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

Toggle widgets NTP #503

Merged
merged 3 commits into from
Jul 3, 2019
Merged

Toggle widgets NTP #503

merged 3 commits into from
Jul 3, 2019

Conversation

imptrx
Copy link
Contributor

@imptrx imptrx commented Jun 20, 2019

Spec: brave/brave-browser#4510

Changes

  • Added more options in the settings menu to toggle the showing of widgets
  • Each of the three widgets (Stats, Clock, TopSites) will show/hide from the menu
  • Quick View:
    toggle-widgets

Test plan

  • Ensure menu opens and closes properly
  • Ensure widgets display properly independently of whether the other widgets are being shown
  • Ensure widgets can toggle from the menu
Link / storybook path to visual changes

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:
      Add setting options to toggle NTP widgets brave-core#2762
    • 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
Copy link
Contributor Author

imptrx commented Jun 20, 2019

For reviewers: Reasoning around implementing the show/hide via HOC logic is that we will be extending our widgets with additional features later.(i.e hover state, right click mini menu etc)
See Spec

@imptrx imptrx self-assigned this Jun 20, 2019
@imptrx imptrx requested a review from cezaraugusto June 20, 2019 23:11
@imptrx imptrx changed the title Toggle widgets Toggle widgets NTP Jun 20, 2019
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.

++

@@ -66,3 +68,5 @@ export class Clock extends React.PureComponent<{}, ClockState> {
)
}
}

export const ClockWidget = createWidget(Clock)
Copy link
Member

Choose a reason for hiding this comment

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

Just a nit to say that this (and some other things in this PR) breaks backwards compatibility with the brave-core consumer, so any gap between this PR being merged with brave-ui's sha that brave-core is using, and the sister brave-core PR being merged, will result in broken builds. But the remedy for now is just to make sure they are merged very quickly together :-)

@imptrx imptrx merged commit a833be9 into master Jul 3, 2019
@imptrx imptrx deleted the toggle-widgets branch July 3, 2019 18:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants