Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Remove tab icons for about:newtab and about:blank #5845

Merged
merged 1 commit into from
Nov 27, 2016
Merged

Remove tab icons for about:newtab and about:blank #5845

merged 1 commit into from
Nov 27, 2016

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Nov 25, 2016

  • 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).

After tweaking the UI for tabs with no icon, it also addressed a fix for #5431

Fix #5776
Fix #5431

Auditors: @bsclifton

/cc @BradRichter could you checkout and see if the fix included here for #5431 fits our need?

Screenshot for #5431:
rsz_close_tab_icon

Test Plan (#5776):

  • Automated test should pass
  • npm run test -- --grep="tabs with icons"

Manual test (Test Plan for #5431):

  • Open 10 tabs with icons
  • Resize width to minimum possible
  • Close button should stay fit to its parent

After tweaking the UI for tabs with no icon, it also addressed a fix for #5431

Fix #5776
Fix #5431

Auditors: @bsclifton

/cc @BradRichter could you checkout and see if the fix included here for #5431 fits our need?

Test Plan (#5776):

* Automated test should pass

Manual test (Test Plan for #5776):

* Open 10 tabs with icons
* Resize width to minimum possible
* Close button should stay fit to its parent
@bsclifton
Copy link
Member

Manually tested; looks and works great! I was a little scared of the tab fix (the disable button issue), but it worked good in every scenario that I tried 😄

@bsclifton bsclifton merged commit dcc0a7a into brave:master Nov 27, 2016
@bsclifton
Copy link
Member

Stay tuned; this PR may be causing an issue... Will be looking into it, trying to confirm. The issue can be seen here:
image

I suspected it may be this because of the change w/ the close button

@luixxiul
Copy link
Contributor

@cezaraugusto would you please update the manual test plan for #5431 explained in the first post, if it is no longer valid?

@bbondy
Copy link
Member

bbondy commented Jan 16, 2017

about:blank should have favicon on the tab btw, I think it was changed after this was landed because it caused a lot of UI jank witih loading icon for delay loaded tabs.

@cezaraugusto
Copy link
Contributor Author

@luixxiul test plan for #5431 is described on #5918, but it's still a WIP. It supersed all tab's icon related issues but still needs some tweaks to be mergeable. I'll follow up your request about #5431 and update it accordingly once PR is ready.

@bbondy currently about:blank has the default paper icon (but newtab has not). If your take is to have a real favicon (like Brave logo) lmk and I'll take the work.

@cezaraugusto cezaraugusto deleted the feature/newtab/5776 branch July 25, 2017 07:28
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.

Newtab should not have an empty favicon Disable close button on tabs if tab size is too small
4 participants