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

Bookmarking a site on new tab gridSites change its position #5413

Closed
cezaraugusto opened this issue Nov 4, 2016 · 8 comments · Fixed by #5583 or #12470
Closed

Bookmarking a site on new tab gridSites change its position #5413

cezaraugusto opened this issue Nov 4, 2016 · 8 comments · Fixed by #5583 or #12470
Assignees
@cezaraugusto
Labels
bug feature/newtab QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/include
Milestone
0.20.x (Release C...

Comments

@cezaraugusto
Copy link
Contributor

cezaraugusto commented Nov 4, 2016 •
edited by bsclifton
Loading

Test plan

See #12470

Original issue description

Did you search for similar issues before submitting this one?
yes
Describe the issue you encountered:
When you bookmark a tile on newtab's grid, the site jumps its position to the first position.

Expected behavior:
Bookmarking a site should take no action on topSites grid

  • Platform (Win7, 8, 10? macOS? Linux distro?): n/a

  • Brave Version: n/a

  • Steps to reproduce:

    1. Go to new tab page
    2. Visit some sites
    3. Take one site on i.e. position 3
    4. Notice it 'jumps' to first position

  • Screenshot if needed: n/æ

  • Any related issues: n/a
@bsclifton
Copy link
Member

Possibly related to re-ordering done here (which may not be needed, just like the sanitize in newtabs.js removed it)
https://github.com/cezaraugusto/browser-laptop/blob/669ff355c34de09bdb4bcdafd88df1c15319e97c/app/common/state/aboutNewTabState.js#L13

@bsclifton bsclifton added this to the 0.12.10 release milestone Nov 9, 2016
@bsclifton bsclifton assigned bsclifton and unassigned cezaraugusto Nov 13, 2016
@bsclifton bsclifton mentioned this issue Nov 13, 2016
Merged
4 tasks
@alexwykoff alexwykoff mentioned this issue Nov 14, 2016
Closed
bbondy pushed a commit that referenced this issue Nov 14, 2016
@bsclifton @bbondy
Refactoring, Bug fixes, and tests for top sites on about:newtab
ec3342c 
Bookmarking items does not affect position
Fixes #5413

Sites are ordered by most visited (count) DESC
Fixes #5322

Groundwork laid for #5565,
but the task is unfinished as-is.

-----
These commits moving existing logic from newtab.js into the session helper.
Tests were then added and I manually tested each scenario and tried to make
sure tests cover those. Things which are not covered are marked with  TODO(bsclifton)

The important thing:
appState.about.newtab.sites no longer persists a separate copy of the sites array.
It instead uses the location/partion of items saved to lookup the real object from appState.sites.
-----

Auditors: @cezaraugusto, @bbondy
This was referenced Nov 14, 2016
Closed
Closed
@luixxiul luixxiul mentioned this issue Nov 14, 2016
Closed
@cezaraugusto
Copy link
Contributor Author

Reopening as still an issue after #5583

@cezaraugusto cezaraugusto reopened this Nov 14, 2016
@bsclifton
Copy link
Member

@cezaraugusto is this still an issue? Also, I'm curious about #5337

I wasn't sure after we talked today if there were outstanding issues? The behavior may have seemed different, but it's because of how it sorts. The pinning should be working as expected

@bbondy
Copy link
Member

bbondy commented Nov 15, 2016

Moving to 0.12.10

@bbondy bbondy modified the milestones: 0.12.10 release , 0.12.9dev Nov 15, 2016
@bsclifton
Copy link
Member

Moving to 0.12.11

@bsclifton bsclifton modified the milestones: 0.12.11, 0.12.10 release Nov 17, 2016
@cezaraugusto cezaraugusto mentioned this issue Nov 21, 2016
Closed
@bsclifton
Copy link
Member

bsclifton commented Nov 27, 2016 •
edited
Loading

@cezaraugusto did you ever submit a fix for this? I remember we talked about how bookmarking may update the lastAccessedTime field (which would cause this)

@cezaraugusto
Copy link
Contributor Author

@bsclifton nope but first item of my list now

@bbondy bbondy modified the milestones: 0.13.2, 0.13.0 Dec 1, 2016
@bsclifton bsclifton removed their assignment Dec 6, 2016
@bsclifton bsclifton mentioned this issue Dec 12, 2016
Closed
@cezaraugusto
Copy link
Contributor Author

cezaraugusto commented Dec 13, 2016 •
edited
Loading

Self notes:

oldSiteDetail is being ignored while defining lastAccessedTime and adding visit count is not setting counter to bookmarked sites.

Current implementation for a fix still breaks actual tests.

This issue is cross-related to #6160

@bsclifton bsclifton mentioned this issue Dec 14, 2016
Merged
3 tasks
@bbondy bbondy modified the milestones: 0.13.5, 0.13.6 Feb 15, 2017
@bsclifton bsclifton added this to the Triage Backlog milestone Nov 27, 2017
@cezaraugusto cezaraugusto mentioned this issue Jan 3, 2018
Merged
@bsclifton bsclifton modified the milestones: Triage Backlog, 0.20.x (Beta Channel) Jan 3, 2018
This was referenced Jan 9, 2018
Closed
Closed
Closed
@kjozwiak kjozwiak mentioned this issue Jan 31, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@cezaraugusto cezaraugusto

Labels
bug feature/newtab QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/include
Projects
None yet
Milestone
0.20.x (Release Channel)
6 participants
@bbondy @luixxiul @cezaraugusto @bsclifton @srirambv @LaurenWags