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

Tweak tabtitle threshold w/ secondary icons on small tabs #9512

Merged
merged 1 commit into from
Jun 19, 2017

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Jun 16, 2017

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.

Auditors: @bsclifton
Close #9466

Test Plan:

covered by automated tests

npm run test -- --grep="Tabs content - NewSessionIcon should not show icon if breakpoint is medium"
npm run test -- --grep="Tabs content - PrivateIcon should not show icon if breakpoint is medium"
npm run test -- --grep="Tabs content - Title should show text if breakpoint is mediumSmall and tab is not active"
npm run test -- --grep="Tabs content - Title should not show text if breakpoint is mediumSmall and tab is active"

Manual Test:

  1. Have enough private/new session/default tabs.
  2. Resize window
  3. Title should not truncate with secondary icons
  4. Title should be aesthetically ok if there are no secondary icons

Reviewer Checklist:

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

Auditors: @bsclifton
Close #9466
Test Plan: covered by automated tests
@cezaraugusto cezaraugusto added this to the 0.17.x (Beta Channel) milestone Jun 16, 2017
@cezaraugusto cezaraugusto self-assigned this Jun 16, 2017
@cezaraugusto cezaraugusto requested a review from bsclifton June 16, 2017 03:59
@bsclifton
Copy link
Member

@cezaraugusto this is ready for review, right?

@bsclifton bsclifton requested a review from NejcZdovc June 18, 2017 18:51
@@ -152,7 +152,8 @@ const tabContentState = {
// If closeIcon is fixed then there's no room for another icon
!tabContentState.hasFixedCloseIcon(state, frameKey) &&
// completely hide it for small sizes
!hasBreakpoint(frame.get('breakpoint'), ['mediumSmall', 'small', 'extraSmall', 'smallest'])
!hasBreakpoint(frame.get('breakpoint'),
['medium', 'mediumSmall', 'small', 'extraSmall', 'smallest'])
Copy link
Contributor

@NejcZdovc NejcZdovc Jun 18, 2017

Choose a reason for hiding this comment

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

I checkout my PR that refactored this function and I see that medium was not there before either. How come that we need to add it now? Was this working correctly before in previous versions or never?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the feature works as expected but turns out to have bad UI. It wasn't introduced in your PR.

@cezaraugusto
Copy link
Contributor Author

@bsclifton yes ready. It wasn't a bug before and wasn't a regression as well but given the bad look and feel this tweak is needed. I'll label needs QA to double-check as well, thanks.

Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

Tests are passing and changes look good 👍

@NejcZdovc
Copy link
Contributor

NejcZdovc commented Jun 19, 2017

Not sure if this is part of this PR or not (if not let's create a issue for it), but I think that it would be good to fix this one as well.

Open some empty new session tabs and try to resize. I think that we should display session icon with a number if there is no fav icon and not an empty space as we do for a regular new tab.

big:
image

medium:
image

small:
image

@bsclifton bsclifton merged commit 07166c7 into master Jun 19, 2017
@bsclifton bsclifton deleted the tabsbar/9466 branch June 19, 2017 16:57
bsclifton added a commit that referenced this pull request Jun 19, 2017
Tweak tabtitle threshold w/ secondary icons on small tabs
bsclifton added a commit that referenced this pull request Jun 19, 2017
Tweak tabtitle threshold w/ secondary icons on small tabs
@cezaraugusto
Copy link
Contributor Author

@NejcZdovc re #9512 (comment) I would go adding paper icon instead. It is the default for websites with broken favicons. Maybe just left the title, but I prefer the former. Anyway, your assumption is valid given empty tab is not pleasing.

/cc @bradleyrichter for the rescue

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