diff --git a/app/browser/api/topSites.js b/app/browser/api/topSites.js index f960ac1e4f8..dfe0c905ece 100644 --- a/app/browser/api/topSites.js +++ b/app/browser/api/topSites.js @@ -20,7 +20,12 @@ let minAccessOfTopSites const staticData = Immutable.fromJS(newTabData.topSites) const isPinned = (state, siteKey) => { - return aboutNewTabState.getPinnedTopSites(state).find(site => site.get('key') === siteKey) + return aboutNewTabState.getPinnedTopSites(state).some(site => { + if (!site || !site.get) { + return false + } + return site.get('key') === siteKey + }) } const isIgnored = (state, siteKey) => { @@ -116,6 +121,7 @@ const getTopSiteData = () => { if (sites.size < 18) { const preDefined = staticData + // TODO: this doesn't work properly .filter((site) => { return !isPinned(state, site.get('key')) && !isIgnored(state, site.get('key')) }) @@ -126,7 +132,27 @@ const getTopSiteData = () => { sites = sites.concat(preDefined) } - appActions.topSiteDataAvailable(sites) + let gridSites = aboutNewTabState.getPinnedTopSites(state).map(pinned => { + // do not allow duplicates + if (pinned) { + sites = sites.filter(site => site.get('key') !== pinned.get('key')) + } + // topsites are populated once user visit a new site. + // pinning a site to a given index is a user decision + // and should be taken as priority. If there's an empty + // space we just fill it with visited sites. Otherwise + // fallback to the pinned item. + if (!pinned) { + const firstSite = sites.first() + sites = sites.shift() + return firstSite + } + return pinned + }) + + gridSites = gridSites.filter(site => site != null) + + appActions.topSiteDataAvailable(gridSites) } /** diff --git a/app/common/state/aboutNewTabState.js b/app/common/state/aboutNewTabState.js index ae16b8044c3..e322a3028d9 100644 --- a/app/common/state/aboutNewTabState.js +++ b/app/common/state/aboutNewTabState.js @@ -4,19 +4,27 @@ const Immutable = require('immutable') const {makeImmutable} = require('./immutableUtil') - +const topSites = require('../../browser/api/topSites') +const newTabData = require('../../../js/data/newTabData') /** * topSites are defined by users. Pinned sites are attached to their positions * in the grid, and the non pinned indexes are populated with newly accessed sites */ - +const defaultPinnedSite = Immutable.fromJS(newTabData.pinnedTopSites) const aboutNewTabState = { getSites: (state) => { return state.getIn(['about', 'newtab', 'sites']) }, getPinnedTopSites: (state) => { - return state.getIn(['about', 'newtab', 'pinnedTopSites'], Immutable.List()) + // add same number as fallback to avoid race condition on startup + const maxEntries = topSites.aboutNewTabMaxEntries || 100 + + // we need null spaces in order to proper pin a topSite in the right position. + // so let's set it to the same number as max new tab entries. + return state + .getIn(['about', 'newtab', 'pinnedTopSites'], Immutable.List()) + .setSize(maxEntries) }, getIgnoredTopSites: (state) => { @@ -28,7 +36,11 @@ const aboutNewTabState = { if (!props) { return state } - + // list is only empty if there's no pinning interaction. + // in this case we include the default pinned top sites list + if (state.getIn(['about', 'newtab', 'pinnedTopSites']).isEmpty()) { + state = state.setIn(['about', 'newtab', 'pinnedTopSites'], defaultPinnedSite) + } state = state.mergeIn(['about', 'newtab'], props.newTabPageDetail) return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) }, diff --git a/app/sessionStore.js b/app/sessionStore.js index b962f0e3d12..eee433a6d05 100644 --- a/app/sessionStore.js +++ b/app/sessionStore.js @@ -32,7 +32,6 @@ const windowState = require('./common/state/windowState') // Utils const locale = require('./locale') -const {pinnedTopSites} = require('../js/data/newTabData') const {defaultSiteSettingsList} = require('../js/data/siteSettingsList') const filtering = require('./filtering') const autofill = require('./autofill') @@ -1007,7 +1006,7 @@ module.exports.defaultAppState = () => { gridLayoutSize: 'small', sites: [], ignoredTopSites: [], - pinnedTopSites: pinnedTopSites + pinnedTopSites: [] }, welcome: { showOnLoad: !['test', 'development'].includes(process.env.NODE_ENV) diff --git a/js/about/newtab.js b/js/about/newtab.js index e63d0d97cfa..daf056ccae5 100644 --- a/js/about/newtab.js +++ b/js/about/newtab.js @@ -97,7 +97,7 @@ class NewTabPage extends React.Component { } get pinnedTopSites () { - return this.state.newTabData.getIn(['newTabDetail', 'pinnedTopSites'], Immutable.List()) + return this.state.newTabData.getIn(['newTabDetail', 'pinnedTopSites'], Immutable.List()).setSize(100) } get ignoredTopSites () { @@ -109,20 +109,18 @@ class NewTabPage extends React.Component { } isPinned (siteKey) { - return this.pinnedTopSites.find(site => site.get('key') === siteKey) + return this.pinnedTopSites.some(site => { + if (!site || !site.get) { + return false + } + return site.get('key') === siteKey + }) } get gridLayout () { const sizeToCount = {large: 18, medium: 12, small: 6} const count = sizeToCount[this.gridLayoutSize] - - let sites = this.pinnedTopSites.take(count) - - if (sites.size < count) { - sites = sites.concat(this.topSites.take(count - sites.size)) - } - - return sites + return this.topSites.take(count) } showNotification () { @@ -178,16 +176,26 @@ class NewTabPage extends React.Component { } onPinnedTopSite (siteKey) { + let sites = this.topSites let pinnedTopSites = this.pinnedTopSites - const siteProps = this.topSites.find(site => site.get('key') === siteKey) - if (this.isPinned(siteKey)) { - pinnedTopSites = pinnedTopSites.filter(site => site.get('key') !== siteKey) + const siteProps = sites.find(site => site.get('key') === siteKey) + + const currentSiteIndex = sites.findIndex(site => site.get('key') === siteKey) + const currentPinnedSiteIndex = pinnedTopSites + .findIndex(site => site && site.get('key') === siteKey) + + // ensure pinned sites are pinned in the right order when pinned + // if not pinned, pin and attach it to its position + if (!this.isPinned(siteKey)) { + pinnedTopSites = pinnedTopSites.splice(currentSiteIndex, 1, siteProps) } else { - pinnedTopSites = pinnedTopSites.push(siteProps) + pinnedTopSites = pinnedTopSites.splice(currentPinnedSiteIndex, 1, null) + sites = sites.splice(currentPinnedSiteIndex, 1, siteProps) + aboutActions.setNewTabDetail({sites}, true) } - aboutActions.setNewTabDetail({pinnedTopSites: pinnedTopSites}, true) + aboutActions.setNewTabDetail({pinnedTopSites}, true) } onIgnoredTopSite (siteKey) { diff --git a/test/unit/app/browser/api/topSitesTest.js b/test/unit/app/browser/api/topSitesTest.js index 9faaf1d9fa8..9babd794e4c 100644 --- a/test/unit/app/browser/api/topSitesTest.js +++ b/test/unit/app/browser/api/topSitesTest.js @@ -154,38 +154,28 @@ describe('topSites api', function () { ]) it('respects position of pinned items when populating results', function () { - const allPinned = Immutable.fromJS([site1, site4]) - const stateWithPinnedSites = defaultAppState + const allPinned = Immutable.fromJS([null, site2, null, site4]) + let stateWithPinnedSites = defaultAppState .set(STATE_SITES.HISTORY_SITES, generateMap(site1, site2, site3, site4)) .setIn(['about', 'newtab', 'pinnedTopSites'], allPinned) this.topSites.calculateTopSites(stateWithPinnedSites) // checks: - // - pinned item are in their expected order + // - pinned item are in their expected order (site 2 at i-1 and site4 at i-3) // - unpinned items fill the rest of the spots (starting w/ highest # visits first) + this.topSites.calculateTopSites(stateWithPinnedSites) getStateValue = stateWithPinnedSites this.clock.tick(calculateTopSitesClockTime) assert.equal(this.appActions.topSiteDataAvailable.callCount, 1) const newSitesData = this.appActions.topSiteDataAvailable.getCall(0).args[0] - const expectedSites = Immutable.fromJS([ - { - location: 'https://example3.com/', - title: 'sample 3', - parentFolderId: 0, - count: 23, - lastAccessedTime: 123, - bookmarked: false, - key: 'https://example3.com/|0' - }, - { - location: 'https://example2.com/', - title: 'sample 2', - parentFolderId: 0, - count: 5, - bookmarked: false, - key: 'https://example2.com/|0' - } - ]) - assert.deepEqual(newSitesData.toJS(), expectedSites.concat(staticNewData).toJS()) + + // assert that first site is populated + assert.deepEqual(newSitesData.get(0).isEmpty(), false) + // assert that site 2 is at i-1 as planned + assert.deepEqual(newSitesData.get(1), site2) + // assert that second site is populated + assert.deepEqual(newSitesData.get(2).isEmpty(), false) + // assert that site 4 is at i-3 as planned + assert.deepEqual(newSitesData.get(3), site4) }) it('only includes one result for a domain (the one with the highest count)', function () { @@ -294,7 +284,7 @@ describe('topSites api', function () { assert.deepEqual(newSitesData.toJS(), expectedSites.concat(staticNewData).toJS()) }) - it('only returns the last `maxSites` results', function () { + it('only returns the last maxSites results', function () { const maxSites = this.topSites.aboutNewTabMaxEntries let tooManySites = Immutable.Map() for (let i = 0; i < maxSites + 1; i++) { diff --git a/test/unit/app/browser/reducers/aboutNewTabsReducerTest.js b/test/unit/app/browser/reducers/aboutNewTabsReducerTest.js index 52d13070420..6a370672637 100644 --- a/test/unit/app/browser/reducers/aboutNewTabsReducerTest.js +++ b/test/unit/app/browser/reducers/aboutNewTabsReducerTest.js @@ -50,7 +50,7 @@ describe('aboutNewTabReducerTest', function () { const initialState = Immutable.fromJS({ settings: { }, about: { - newtab: { } + newtab: { pinnedTopSites: [] } } }) it('gets a value from default settings when nothing is set', () => {