Tabs bottom border radius and separator UI #422

petemill · 2018-09-07T20:49:35Z

Brave design spec shows no bottom border radius on tabs.
This patching method, although not great, is simple compared to the amount of code which would need to be copied over and only slightly modified, since bottom border radius is calculated in the middle of a long drawing function.

Feedback appreciated.

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.
Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Test Plan:

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

bbondy · 2018-09-08T03:17:53Z

removed label on PR until it's landed on that branch, I think that's preferable to see fast when things are merged and not accidentally miss anything. I do this typically from the closed PR page.

petemill · 2018-09-10T19:33:49Z

Rebasing and testing on latest master / C70

petemill · 2018-09-11T03:58:01Z

This is ready for review now. @bbondy @bridiver I also tagged you for review in case you had an opinion on this new patching method (making the chromium namespace non-anonymous so that it can be accessed by the brave override class).

simonhong · 2018-09-11T05:25:00Z

When we need to override some original method and our new method needs many anonymous functions in original file, how about below in upstream file?

#if defined(BRAVE_CHROMIUM_BUILD)
void Tab::PaintSeparators(gfx::Canvas* canvas) {
  ...
  our implementation
  ...
}
#else
void Tab::PaintSeparators(gfx::Canvas* canvas) {
  ...
  original implementation
  ...
}
#endif

petemill · 2018-09-11T22:10:17Z

@simonhong that is valid, but it puts some of our customized logic mixed in with the chromium source, whilst we may have some other logic in the brave modules. The method currently in this PR is a proposal to keep the logic changes in the brave modules to avoid confusion. If we still prefer putting the function in the patched file over de-anonymizing the namespace, then sure - I'll make that change.

browser/ui/views/tabs/brave_tab.cc

simonhong · 2018-09-11T22:46:26Z

@petemill , Yep, my suggestion is just another option :)

Fix brave/brave-browser#962 Brave design spec shows no bottom border radius on tabs. This patching method, although not great, is simple compared to the amount of code which would need to be copied over and only slightly modified.

petemill · 2018-09-13T02:40:37Z

@simonhong addressed all feedback now - thanks. Decided to keep in the de-anonymizing namespace option after speaking to other team members due to the less worse impact on future rebasing.

simonhong

LGTM with nits. 👍

simonhong · 2018-09-13T12:46:53Z

browser/ui/views/tabs/brave_tab.cc

+  flags.setColor(separator_color(separator_opacities.right));
+  canvas->DrawRoundRect(
+                     trailing_separator_bounds, scaled_separator_radius, flags);
+}


nit: last empty line.

simonhong · 2018-09-13T12:47:05Z

browser/ui/views/tabs/brave_tab.h

+#include "ui/gfx/geometry/rect.h"
+
+namespace chrome_browser_ui_views_tabs_tab_cc {
+  const gfx::RectF ScaleAndAlignBounds(const gfx::Rect& bounds,


nit: indent

simonhong · 2018-09-13T12:47:20Z

browser/ui/views/tabs/brave_tab.h

+}
+
+class BraveTab : public Tab {
+  public:


nit: indent

Address brave/brave-browser#962

bsclifton

++

Tabs bottom border radius and separator UI

petemill · 2018-09-13T16:42:14Z

0.55.x f1d811d

bbondy · 2018-09-14T17:16:17Z

Added merged/0.55.x label

v5 storybook update

petemill added UI 0.55.x labels Sep 7, 2018

petemill self-assigned this Sep 7, 2018

petemill requested review from bridiver and bbondy and removed request for bridiver and bbondy September 7, 2018 20:49

petemill added the pr/work-in-progress label Sep 7, 2018

petemill changed the title ~~Remove bottom border radius from Tabs~~ [WIP] Remove bottom border radius from Tabs Sep 7, 2018

bbondy changed the title ~~[WIP] Remove bottom border radius from Tabs~~ WIP: Remove bottom border radius from Tabs Sep 8, 2018

bbondy removed the 0.55.x label Sep 8, 2018

petemill force-pushed the ui/tab-bottom-corners branch from f3842b1 to df6e36b Compare September 10, 2018 18:38

petemill changed the title ~~WIP: Remove bottom border radius from Tabs~~ Tabs bottom border radius and separator UI Sep 10, 2018

petemill changed the title ~~Tabs bottom border radius and separator UI~~ WIP: Tabs bottom border radius and separator UI Sep 10, 2018

petemill force-pushed the ui/tab-bottom-corners branch 2 times, most recently from 8f7c159 to 6d36bf1 Compare September 11, 2018 03:56

petemill removed the pr/work-in-progress label Sep 11, 2018

petemill requested review from bbondy, bridiver and simonhong September 11, 2018 03:56

simonhong reviewed Sep 11, 2018

View reviewed changes

browser/ui/views/tabs/brave_tab.cc Outdated Show resolved Hide resolved

browser/ui/views/tabs/brave_tab.cc Show resolved Hide resolved

simonhong changed the title ~~WIP: Tabs bottom border radius and separator UI~~ Tabs bottom border radius and separator UI Sep 12, 2018

petemill added 2 commits September 12, 2018 19:14

Remove bottom border radius from Tabs

ca55d46

Fix brave/brave-browser#962 Brave design spec shows no bottom border radius on tabs. This patching method, although not great, is simple compared to the amount of code which would need to be copied over and only slightly modified.

Remove patch for obsolete TabStrip UI code

7738add

petemill force-pushed the ui/tab-bottom-corners branch from 6d36bf1 to 6c10e41 Compare September 13, 2018 02:38

simonhong previously approved these changes Sep 13, 2018

View reviewed changes

Brave tab separator UI

de337bc

Address brave/brave-browser#962

petemill dismissed simonhong’s stale review via de337bc September 13, 2018 16:29

petemill force-pushed the ui/tab-bottom-corners branch from 6c10e41 to de337bc Compare September 13, 2018 16:29

bsclifton approved these changes Sep 13, 2018

View reviewed changes

petemill merged commit 0d1b737 into master Sep 13, 2018

petemill deleted the ui/tab-bottom-corners branch September 13, 2018 16:41

petemill added a commit that referenced this pull request Sep 13, 2018

Merge pull request #422 from brave/ui/tab-bottom-corners

f1d811d

Tabs bottom border radius and separator UI

bbondy added the merged/0.55.x label Sep 14, 2018

bbondy added this to the 0.55.x - Release milestone Jan 14, 2019

cezaraugusto added a commit that referenced this pull request Jul 17, 2019

Imported from brave-ui: Merge pull request #422 from brave/v5-storybook

e2fb393

v5 storybook update

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tabs bottom border radius and separator UI #422

Tabs bottom border radius and separator UI #422

petemill commented Sep 7, 2018

bbondy commented Sep 8, 2018

petemill commented Sep 10, 2018

petemill commented Sep 11, 2018

simonhong commented Sep 11, 2018 •

edited

Loading

petemill commented Sep 11, 2018

simonhong commented Sep 11, 2018

petemill commented Sep 13, 2018

simonhong left a comment

simonhong Sep 13, 2018

simonhong Sep 13, 2018

simonhong Sep 13, 2018

bsclifton left a comment

petemill commented Sep 13, 2018

bbondy commented Sep 14, 2018

Tabs bottom border radius and separator UI #422

Tabs bottom border radius and separator UI #422

Conversation

petemill commented Sep 7, 2018

Submitter Checklist:

Test Plan:

Reviewer Checklist:

bbondy commented Sep 8, 2018

petemill commented Sep 10, 2018

petemill commented Sep 11, 2018

simonhong commented Sep 11, 2018 • edited Loading

petemill commented Sep 11, 2018

simonhong commented Sep 11, 2018

petemill commented Sep 13, 2018

simonhong left a comment

Choose a reason for hiding this comment

simonhong Sep 13, 2018

Choose a reason for hiding this comment

simonhong Sep 13, 2018

Choose a reason for hiding this comment

simonhong Sep 13, 2018

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

petemill commented Sep 13, 2018

bbondy commented Sep 14, 2018

simonhong commented Sep 11, 2018 •

edited

Loading