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.

Sign up for GitHub

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

Jump to bottom

Reader: Pretty URLs for some feeds #832

Merged
merged 4 commits into from
Dec 15, 2015
Merged

Reader: Pretty URLs for some feeds #832

merged 4 commits into from
Dec 15, 2015

Conversation

mjangda
Copy link
Member

@mjangda mjangda commented Nov 26, 2015

The primary goal of this PR is to redirect full site/feed URLs for Discover to the pretty version (i.e. /discover) but this refactors the URL generation for streams and makes it easier to add other pretty URLs in the future (/longreads?).

Testing

  • Verify that /read/blog/feed/12733228 and /read/blog/id/53424024 both redirect to /discover.
  • Verify that all streams load correctly.
  • Verify that clicking a post and going back work as expected.
  • Verify that clicking on a Site Header in a stream loads that site/feed.

@mjangda mjangda added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Reader The reader site on Calypso. labels Nov 26, 2015
mtias
mtias reviewed Nov 26, 2015
View reviewed changes
client/reader/controller.js
@@ -19,7 +19,8 @@ var i18n = require( 'lib/mixins/i18n' ),
i18n = require( 'lib/mixins/i18n' ),
TitleStore = require( 'lib/screen-title/store' ),
titleActions = require( 'lib/screen-title/actions' ),
FeedSubscriptionActions = require( 'lib/reader-feed-subscriptions/actions' );
FeedSubscriptionActions = require( 'lib/reader-feed-subscriptions/actions' ),
urlHelper = require( 'reader/url-helper' );
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to include this resource in lib/route/path? There are some similar helpers there. Or even a new lib/route/reader.js.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea moving it to lib/route/reader.js. @bluefuton or @blowery any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 sounds like a good place for it to live :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved in 22356af and added tests in d078858

@mjangda
Add URLHelper module that generates reader URLs
ec68204 
Also make it easier to add pretty aliases for streams like `/discover`
@mjangda
Use urlHelper module to generate stream URLs
391428c 
Across various places in the reader codebase where we were hard-coding
the stream paths.
@mjangda
Implement stream redirects in reader controller
674d92d 
If a feed/site has a pretty URL, we should redirect to that instead of
showing the full ID-laden URL.
@mjangda mjangda force-pushed the update/pretty-reader-urls branch from a948a09 to d078858 Compare December 2, 2015 01:39
@blowery
Copy link
Contributor

blowery commented Dec 3, 2015

Maybe reader/route or push it into reader/index with the rest of the routing info, instead of lib/route/reader. Why would reader routing info need to break out?

@blowery blowery added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 5, 2015
@mjangda
Move url-helper to reader/route
7e80cad 
Also adds tests for the helper.
@mjangda mjangda force-pushed the update/pretty-reader-urls branch from d078858 to 7e80cad Compare December 7, 2015 23:14
@mjangda mjangda added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Dec 7, 2015
@mjangda
Copy link
Member Author

mjangda commented Dec 7, 2015

Cool, moved to reader/route.

@mtias
Copy link
Member

mtias commented Dec 8, 2015

@blowery I thought the file was about formatting links to use in views, in which case it makes sense as a /lib resource to me, since that's where we are pushing all utilities to be in. (With group sections in client just about views, route declarations, and controllers.) I also see there are files like utils.js, x-post-helper.js, etc in the root of reader.

@bluefuton
Copy link
Contributor

Looks good to me @mjangda 👍

@mtias @blowery do we want the view helpers to move to lib/reader-routes perhaps?

@mtias
Copy link
Member

mtias commented Dec 15, 2015

@bluefuton I don't want to hold this PR because of figuring out where to place things. We can continue the discussion in a separate issue. I do think it would make our lives easier to place all these utils in lib as a general rule. It means if we need to construct a link to the reader from somewhere else in the application we know where to look for helpers.

mjangda added a commit that referenced this pull request Dec 15, 2015
@mjangda
Merge pull request #832 from Automattic/update/pretty-reader-urls
1123849 
Reader: Pretty URLs for some feeds
@mjangda mjangda merged commit 1123849 into master Dec 15, 2015
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 15, 2015
@mjangda mjangda deleted the update/pretty-reader-urls branch December 15, 2015 20:14
@bluefuton bluefuton added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Dec 15, 2015
@ajesse11x ajesse11x mentioned this pull request Mar 8, 2021
Open
@ajesse11x ajesse11x mentioned this pull request Nov 4, 2022
Open
@awaisab172 awaisab172 mentioned this pull request Nov 5, 2022
Open
@ajesse11x ajesse11x mentioned this pull request Dec 25, 2022
Open
@awaisab172 awaisab172 mentioned this pull request Dec 26, 2022
Open
@ajesse11x ajesse11x mentioned this pull request Nov 29, 2023
Open
@ajesse11x ajesse11x mentioned this pull request May 14, 2024
Open
@ajesse11x ajesse11x mentioned this pull request May 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
[Feature] Reader The reader site on Calypso. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mjangda @blowery @mtias @bluefuton @scruffian