Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reader: Filter main feed based on sidebar selection #97123

Merged
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
{
mehmoodak marked this conversation as resolved.
Show resolved Hide resolved
"version": "0.2.0",
"configurations": [
{
"name": "Launch Brave",
"request": "launch",
"type": "chrome",
"url": "http://calypso.localhost:3000",
"webRoot": "${workspaceFolder}/client",
"runtimeExecutable": "/Applications/Brave Browser.app/Contents/MacOS/Brave Browser",
"sourceMapPathOverrides": {
"app://": "${workspaceFolder}/client"
}
},
{
"name": "Launch Chrome",
"request": "launch",
"type": "chrome",
"url": "http://calypso.localhost:3000",
"webRoot": "${workspaceFolder}/client",
"sourceMapPathOverrides": {
"app://": "${workspaceFolder}/client"
}
},
{
"name": "Launch Firefox",
"request": "launch",
"reAttach": true,
"type": "firefox",
"url": "http://calypso.localhost:3000",
"webRoot": "${workspaceFolder}/client",
"pathMappings": [
{
"url": "app:///",
"path": "${workspaceFolder}/client"
}
]
}
]
}
37 changes: 8 additions & 29 deletions client/reader/sidebar/index.jsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { isEnabled } from '@automattic/calypso-config';
import page from '@automattic/calypso-router';
import { hasTranslation } from '@wordpress/i18n';
import closest from 'component-closest';
import i18n, { localize } from 'i18n-calypso';
import { defer, startsWith } from 'lodash';
Expand All @@ -20,7 +19,6 @@ import SidebarSeparator from 'calypso/layout/sidebar/separator';
import ReaderA8cConversationsIcon from 'calypso/reader/components/icons/a8c-conversations-icon';
import ReaderConversationsIcon from 'calypso/reader/components/icons/conversations-icon';
import ReaderDiscoverIcon from 'calypso/reader/components/icons/discover-icon';
import ReaderFollowingIcon from 'calypso/reader/components/icons/following-icon';
import ReaderLikesIcon from 'calypso/reader/components/icons/likes-icon';
import ReaderManageSubscriptionsIcon from 'calypso/reader/components/icons/manage-subscriptions-icon';
import ReaderNotificationsIcon from 'calypso/reader/components/icons/notifications-icon';
Expand Down Expand Up @@ -117,13 +115,6 @@ export class ReaderSidebar extends Component {
} );
};

handleReaderSidebarFollowedSitesClicked = ( event, path ) => {
recordAction( 'clicked_reader_sidebar_followed_sites' );
recordGaEvent( 'Clicked Reader Sidebar Followed Sites' );
this.props.recordReaderTracksEvent( 'calypso_reader_sidebar_followed_sites_clicked' );
this.handleGlobalSidebarMenuItemClick( path );
};

handleReaderSidebarConversationsClicked = ( event, path ) => {
recordAction( 'clicked_reader_sidebar_conversations' );
recordGaEvent( 'Clicked Reader Sidebar Conversations' );
Expand Down Expand Up @@ -174,8 +165,8 @@ export class ReaderSidebar extends Component {
};

renderSidebarMenu() {
const { path, translate, teams, locale } = this.props;
const recentLabelTranslationReady = hasTranslation( 'Recent' ) || locale.startsWith( 'en' );
const { path, translate, teams } = this.props;

return (
<SidebarMenu>
<QueryReaderLists />
Expand All @@ -194,25 +185,13 @@ export class ReaderSidebar extends Component {

<SidebarSeparator />

{ isEnabled( 'reader/recent-feed-overhaul' ) ? (
<li className="sidebar-streams__following">
<ReaderSidebarRecent
onClick={ this.props.toggleFollowingVisibility }
isOpen={ this.props.isFollowingOpen }
path={ path }
/>
</li>
) : (
<SidebarItem
className={ ReaderSidebarHelper.itemLinkClass( '/read', path, {
'sidebar-streams__following': true,
} ) }
label={ recentLabelTranslationReady ? translate( 'Recent' ) : translate( 'Following' ) }
onNavigate={ this.handleReaderSidebarFollowedSitesClicked }
customIcon={ <ReaderFollowingIcon viewBox="-3 0 24 24" /> }
link="/read"
<li className="sidebar-streams__following">
mehmoodak marked this conversation as resolved.
Show resolved Hide resolved
<ReaderSidebarRecent
onClick={ this.props.toggleFollowingVisibility }
isOpen={ this.props.isFollowingOpen }
path={ path }
/>
) }
</li>

<SidebarItem
className={ ReaderSidebarHelper.itemLinkClass( '/discover', path, {
Expand Down
13 changes: 13 additions & 0 deletions client/reader/sidebar/reader-sidebar-recent/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { useDispatch, useSelector } from 'react-redux';
import ExpandableSidebarMenu from 'calypso/layout/sidebar/expandable';
import Favicon from 'calypso/reader/components/favicon';
import ReaderFollowingIcon from 'calypso/reader/components/icons/following-icon';
import { recordAction, recordGaEvent } from 'calypso/reader/stats';
import { recordReaderTracksEvent } from 'calypso/state/reader/analytics/actions';
import getReaderFollowedSites from 'calypso/state/reader/follows/selectors/get-reader-followed-sites';
import { selectSidebarRecentSite } from 'calypso/state/reader-ui/sidebar/actions';
import { AppState } from 'calypso/types';
Expand Down Expand Up @@ -75,6 +77,17 @@ const ReaderSidebarRecent = ( {
if ( ! RECENT_PATH_REGEX.test( path ) ) {
page( '/read' );
}

// Analytics.
if ( feedId ) {
recordAction( 'clicked_reader_sidebar_followed_single_site' );
recordGaEvent( 'Clicked Reader Sidebar Followed Single Site' );
dispatch( recordReaderTracksEvent( 'calypso_reader_sidebar_followed_single_site_clicked' ) );
mehmoodak marked this conversation as resolved.
Show resolved Hide resolved
} else {
recordAction( 'clicked_reader_sidebar_followed_sites' );
recordGaEvent( 'Clicked Reader Sidebar Followed Sites' );
dispatch( recordReaderTracksEvent( 'calypso_reader_sidebar_followed_sites_clicked' ) );
}
};

return (
Expand Down
65 changes: 51 additions & 14 deletions client/reader/stream/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { getReaderOrganizations } from 'calypso/state/reader/organizations/selec
import { getPostByKey } from 'calypso/state/reader/posts/selectors';
import { getBlockedSites } from 'calypso/state/reader/site-blocks/selectors';
import {
clearStream,
requestPage,
selectItem,
selectNextItem,
Expand All @@ -42,6 +43,7 @@ import {
} from 'calypso/state/reader/streams/selectors';
import { viewStream } from 'calypso/state/reader-ui/actions';
import { resetCardExpansions } from 'calypso/state/reader-ui/card-expansions/actions';
import { getSelectedFeedId } from 'calypso/state/reader-ui/sidebar/selectors';
import getCurrentLocaleSlug from 'calypso/state/selectors/get-current-locale-slug';
import getPrimarySiteId from 'calypso/state/selectors/get-primary-site-id';
import isNotificationsOpen from 'calypso/state/selectors/is-notifications-open';
Expand Down Expand Up @@ -126,13 +128,19 @@ class ReaderStream extends Component {
this.setState( { selectedTab: 'sites' } );
};

componentDidUpdate( { selectedPostKey, streamKey } ) {
componentDidUpdate( { selectedPostKey, streamKey, selectedFeedId } ) {
if ( streamKey !== this.props.streamKey ) {
this.props.resetCardExpansions();
this.props.viewStream( streamKey, window.location.pathname );
this.fetchNextPage( {} );
}

if ( selectedFeedId !== this.props.selectedFeedId ) {
this.scrollFeedListToTop();
this.props.clearStream( { streamKey } );
this.fetchNextPage( {}, { ...this.props, stream: null } ); // Stream as null to start fresh pagination.
}

if ( ! keysAreEqual( selectedPostKey, this.props.selectedPostKey ) ) {
// Don't scroll to the post if it was clicked for selection. This causes the scroll to
// propagate into the full post screen the first time you click select an item in the
Expand All @@ -147,6 +155,7 @@ class ReaderStream extends Component {
if ( this.props.shouldRequestRecs ) {
this.props.requestPage( {
streamKey: this.props.recsStreamKey,
feedId: this.props.selectedFeedId,
pageHandle: this.props.recsStream.pageHandle,
localeSlug: this.props.localeSlug,
} );
Expand Down Expand Up @@ -447,8 +456,13 @@ class ReaderStream extends Component {
};

poll = () => {
const { streamKey, localeSlug } = this.props;
this.props.requestPage( { streamKey, isPoll: true, localeSlug: localeSlug } );
const { streamKey, localeSlug, selectedFeedId } = this.props;
this.props.requestPage( {
streamKey,
feedId: selectedFeedId,
isPoll: true,
localeSlug: localeSlug,
} );
};

getPageHandle = ( pageHandle, startDate ) => {
Expand All @@ -461,28 +475,30 @@ class ReaderStream extends Component {
};

fetchNextPage = ( options, props = this.props ) => {
const { streamKey, stream, startDate, localeSlug } = props;
const { streamKey, stream, startDate, localeSlug, selectedFeedId } = props;
if ( options.triggeredByScroll ) {
const pageId = pagesByKey.get( streamKey ) || 0;
pagesByKey.set( streamKey, pageId + 1 );

props.trackScrollPage( pageId );
}
const pageHandle = this.getPageHandle( stream.pageHandle, startDate );
props.requestPage( { streamKey, pageHandle, localeSlug } );
const pageHandle = stream ? this.getPageHandle( stream.pageHandle, startDate ) : null;
props.requestPage( { feedId: selectedFeedId, streamKey, pageHandle, localeSlug } );
};

showUpdates = () => {
const { streamKey } = this.props;
this.props.onUpdatesShown();
this.props.showUpdates( { streamKey } );
// @todo: do we need to shuffle?
// if ( this.props.recommendationsStore ) {
// shufflePosts( this.props.recommendationsStore.id );
// }
if ( this.listRef.current ) {
this.listRef.current.scrollToTop();
this.scrollFeedListToTop();
};

scrollFeedListToTop = () => {
if ( ! this.listRef.current ) {
return;
}

this.listRef.current.scrollToTop();
};

renderLoadingPlaceholders = () => {
Expand Down Expand Up @@ -599,8 +615,15 @@ class ReaderStream extends Component {
};

render() {
const { translate, forcePlaceholders, lastPage, streamHeader, streamKey, selectedPostKey } =
this.props;
const {
translate,
forcePlaceholders,
lastPage,
streamHeader,
streamKey,
selectedPostKey,
selectedFeedId,
} = this.props;
const wideDisplay = this.props.width > WIDE_DISPLAY_CUTOFF;
const isReaderCouncilStream = false; // Disabling banner. Original condition: ( this.props.isDiscoverStream || this.props.streamKey === 'following' );
let { items, isRequesting } = this.props;
Expand All @@ -613,6 +636,18 @@ class ReaderStream extends Component {
isRequesting = true;
}

// Due to infinite scrolling or fast selection, API call responses may include items from previously selected feeds.
// To temporarily address this issue, filtering out items that don't belong to the currently selected feed.
//
// The proper fix here is to cancel the pending API calls after the feed selection is changed.
mehmoodak marked this conversation as resolved.
Show resolved Hide resolved
if ( selectedFeedId ) {
items = items.filter( ( item ) => item.feed_ID === selectedFeedId );

if ( items.length === 0 ) {
isRequesting = true;
}
}

const hasNoPosts = this.isMounted && items.length === 0 && ! isRequesting;

const streamType = getStreamType( streamKey );
Expand Down Expand Up @@ -745,6 +780,7 @@ export default connect(
notificationsOpen: isNotificationsOpen( state ),
stream,
recsStream: getStream( state, recsStreamKey ),
selectedFeedId: getSelectedFeedId( state ),
selectedPostKey: stream.selected,
selectedPost,
lastPage: stream.lastPage,
Expand All @@ -757,6 +793,7 @@ export default connect(
};
},
{
clearStream,
resetCardExpansions,
likePost,
unlikePost,
Expand Down
7 changes: 4 additions & 3 deletions client/state/data-layer/wpcom/read/streams/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ const streamApis = {
*/
export function requestPage( action ) {
const {
payload: { streamKey, streamType, pageHandle, isPoll, gap, localeSlug, page, perPage },
payload: { streamKey, streamType, feedId, pageHandle, isPoll, gap, localeSlug, page, perPage },
} = action;
const api = streamApis[ streamType ];

Expand Down Expand Up @@ -385,15 +385,16 @@ export function requestPage( action ) {
// There is a race condition in switchLocale when retrieving the language file
// The stream request can occur before the language file is loaded, so we need a way to explicitly set the lang in the request
const lang = localeSlug || i18n.getLocaleSlug();
const commonQueryParams = { ...algorithm, feed_id: feedId };

return http( {
method: 'GET',
path: path( { ...action.payload } ),
apiVersion,
apiNamespace: api.apiNamespace ?? null,
query: isPoll
? pollQuery( [], { ...algorithm } )
: query( { ...pageHandle, ...algorithm, number, lang, page }, action.payload ),
? pollQuery( [], commonQueryParams )
: query( { ...commonQueryParams, ...pageHandle, number, lang, page }, action.payload ),
onSuccess: action,
onFailure: action,
} );
Expand Down
4 changes: 2 additions & 2 deletions client/state/data-layer/wpcom/read/streams/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,15 @@ describe( 'streams', () => {

it( 'should set proper params for subsequent fetches', () => {
const pageHandle = { after: 'the robots attack' };
const secondPage = { ...action, payload: { ...action.payload, pageHandle } };
const secondPage = { ...action, payload: { ...action.payload, pageHandle, feedId: 1234 } };
const httpAction = requestPage( secondPage );

expect( httpAction ).toEqual(
http( {
method: 'GET',
path: '/read/following',
apiVersion: '1.2',
query: { ...query, number: PER_FETCH, after: 'the robots attack' },
query: { ...query, feed_id: 1234, number: PER_FETCH, after: 'the robots attack' },
onSuccess: secondPage,
onFailure: secondPage,
} )
Expand Down
42 changes: 0 additions & 42 deletions client/state/reader-ui/sidebar/selectors.js
mehmoodak marked this conversation as resolved.
Show resolved Hide resolved

This file was deleted.

Loading
Loading