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

Moves bookmark text calculation into the state #10325

Merged
merged 1 commit into from
Sep 26, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Aug 7, 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.

Resolves #9517
Resolves #9939

Auditors:

Test Plan:

  1. add so many bookmarks that you see bookmark overflow icon
  2. check if overflow is displayed correctly
  3. open two windows and try to resize them, check if overflow is displayed correctly
  4. try reorder bookmarks from overflow to toolbar and see if overflow is still displayed correctly

Reviewer Checklist:

Tests

  • 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

@NejcZdovc NejcZdovc added this to the 0.21.x (Nightly Channel) milestone Aug 7, 2017
@NejcZdovc NejcZdovc self-assigned this Aug 7, 2017
@luixxiul
Copy link
Contributor

luixxiul commented Aug 7, 2017

let's remove package-lock.json for now.

@NejcZdovc
Copy link
Contributor Author

@luixxiul good catch, thank you

@NejcZdovc NejcZdovc force-pushed the hotfix/#9611-textCalc branch 15 times, most recently from f6ad0c4 to 003c142 Compare August 11, 2017 11:18
@codecov-io
Copy link

codecov-io commented Aug 15, 2017

Codecov Report

Merging #10325 into master will increase coverage by 0.8%.
The diff coverage is 83.33%.

@@            Coverage Diff            @@
##           master   #10325     +/-   ##
=========================================
+ Coverage   53.39%   54.19%   +0.8%     
=========================================
  Files         253      255      +2     
  Lines       21892    22079    +187     
  Branches     3421     3448     +27     
=========================================
+ Hits        11689    11966    +277     
+ Misses      10203    10113     -90
Flag Coverage Δ
#unittest 54.19% <83.33%> (+0.8%) ⬆️
Impacted Files Coverage Δ
app/browser/menu.js 51.92% <ø> (-0.27%) ⬇️
js/actions/windowActions.js 4.85% <ø> (+0.04%) ⬆️
js/constants/appConstants.js 100% <ø> (ø) ⬆️
js/constants/windowConstants.js 100% <ø> (ø) ⬆️
app/common/lib/historyUtil.js 72.63% <0%> (ø) ⬆️
js/stores/windowStore.js 27.8% <0%> (+0.04%) ⬆️
js/actions/appActions.js 12.92% <0%> (-0.27%) ⬇️
app/common/lib/bookmarkFoldersUtil.js 97.05% <100%> (+0.35%) ⬆️
app/common/state/bookmarkFoldersState.js 74.8% <100%> (+7.65%) ⬆️
app/common/lib/bookmarkUtil.js 99.32% <100%> (+44.24%) ⬆️
... and 22 more

@NejcZdovc
Copy link
Contributor Author

Tests added. Tests for

  • bookmarksState
  • bookmarkFoldersState

will be handled in #10310

Copy link
Member

@kjozwiak kjozwiak left a comment

Choose a reason for hiding this comment

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

@NejcZdovc I'm still not seeing the overflow icon at the end of the bookmarks toolbar using the following commit:

commit 1c51823
Author: NejcZdovc nejc.zdovc@3zsistemi.si
Date: Mon Sep 11 12:35:57 2017 +0200

review comments 2

I went through the same STR that I mentioned above and imported my bookmarks. I moved them all into the bookmark toolbar and resized the window several times without seeing the overflow icon. I also went through the following settings via about:preferences#general:

  • enabled Text Only and resized the window several times (no overflow icon)
  • enabled Text and Favicons and resized the window several time (no overflow icon)
  • enabled Favicons only and resized the window several time (no overflow icon)

Quick example:

bookmarkissue

@bbondy
Copy link
Member

bbondy commented Sep 15, 2017

@kjozwiak this is ready for you to re-review next week.

@kjozwiak
Copy link
Member

kjozwiak commented Sep 15, 2017

@bbondy sounds good! I started testing this yesterday but transitioned into the 0.18.36 once the builds were ready. I'll get this finished first thing Monday morning.

Just an FYI, the overflow icon is now appearing correctly but I still wanted to go through a few more tests to make sure there's no regressions.

Copy link
Member

@kjozwiak kjozwiak left a comment

Choose a reason for hiding this comment

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

Used the following PR:

commit e54ce52
Author: NejcZdovc nejc.zdovc@3zsistemi.si
Date: Wed Sep 13 22:52:08 2017 +0200

review commit 3

Possible Issues:

Issue: Favicons not loading:

Favicons are not being loaded when they've been imported and added into the bookmark toolbar. This appears to be a new regression that this PR might have caused as I couldn't reproduce the issue with the following builds:

0.18.36 rev: 7ab85e9 - Couldn't Reproduce
0.19.6 rev: 3be25d4 - Couldn't Reproduce
0.21.0 rev: 2b120a0 - Couldn't Reproduce

bookmarkfavicon

STR:

  • under about:preferences#general, select "Favicons Only"
  • import several bookmarks via about:bookmarks
  • move the bookmarks into the bookmarks toolbar
  • open all those bookmarks in new tabs and you'll notice that the favicons are not updating

Issue: Moving folders from other folders into bookmarks doesn't work

Folders that belong to other folders will not appear under the bookmark toolbar when moved unless the original folder has been removed. This appears to be a new regression that this PR might have caused as I couldn't reproduce the issue with the following builds:

0.18.36 rev: 7ab85e9 - Couldn't Reproduce
0.19.6 rev: 3be25d4 - Couldn't Reproduce
0.21.0 rev: 2b120a0 - Couldn't Reproduce

bookmarkmoving

STR:

  • open about:bookmarks and create a folder named "Test1" under the toolbar
  • create a folder names "Test2" and move it into "Test1"
  • once "Test2" is inside the "Test1" folder, move "Test2" into the toolbar
  • you'll notice it won't be visible
  • delete "Test1" and "Test2" should now be appearing under the toolbar

@NejcZdovc, I'm still going through it but here's two possible issues that you can take a look at so you're not blocked and waiting for me.

@NejcZdovc
Copy link
Contributor Author

@kjozwiak I tried first bug and favicons are not imported on master as well, so it's not limited to this PR. Bug 2 was fixes for folders and bookmarks

Copy link
Member

@kjozwiak kjozwiak left a comment

Choose a reason for hiding this comment

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

Looks good @NejcZdovc! I went through a bunch of different test cases and didn't run into any obvious regressions. I also went through the issues that I mentioned above and ensured those were working correctly as well.

Used the following PR during testing:

Kamils-MBP:browser-laptop kjozwiak$ git log -1
commit 9b414b0
Author: NejcZdovc nejc.zdovc@3zsistemi.si
Date: Mon Aug 7 17:52:26 2017 +0200

Moves bookmark text calculation into the state

Resolves #9517

Auditors:

Test Plan

@bbondy bbondy merged commit 9f6b252 into brave:master Sep 26, 2017
@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017
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.

7 participants