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 Manage Following: present a follow button if user appears to have entered a URL #13715

Merged
merged 9 commits into from
May 8, 2017

Conversation

bluefuton
Copy link
Contributor

As outlined in #12950, present a follow button rather than search results if the user has entered a URL in the search input on Manage Following:

48354c84-1e05-11e7-9cdd-75fcd1c7777a

@bluefuton bluefuton added [Feature] Reader The reader site on Calypso. [Status] In Progress labels May 5, 2017
@bluefuton bluefuton self-assigned this May 5, 2017
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] M Medium sized issue label May 5, 2017
@bluefuton
Copy link
Contributor Author

bluefuton commented May 5, 2017

Todo:

  • prepend protocol if it's missing from siteUrl (follow will fail otherwise)
  • conversely, omit the protocol from the label (so we don't get "Follow http://www.example.com")

@matticbot matticbot added [Size] L Large sized issue and removed [Size] M Medium sized issue labels May 5, 2017
@@ -0,0 +1,43 @@
/**
Copy link
Contributor

@samouri samouri May 8, 2017

Choose a reason for hiding this comment

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

I think we should squint at this file a bit and try to merge it with /lib/url/index.js

isUrl is brand new, but the others looks like they may exist with a different name

@samouri
Copy link
Contributor

samouri commented May 8, 2017

try and do 'you're already following that feed' without a trip to the API?

@blowery merged in a selector that should be able to grab that data! isFollowing( state, { feedUr, feedId, blogId })

@bluefuton
Copy link
Contributor Author

Todo in followup PRs:

@bluefuton bluefuton added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels May 8, 2017
@@ -285,3 +286,30 @@ describe( 'urlToSlug()', () => {
expect( urlWithoutHttp ).to.equal( 'example.com::example::test123' );
} );
} );

describe( 'resemblesUrl()', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

beautiful.
this entire commit is beautiful


// Make sure the query has a protocol - hostname ends up blank otherwise
if ( ! parsedUrl.protocol ) {
parsedUrl = url.parse( 'http://' + query );
Copy link
Contributor

Choose a reason for hiding this comment

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

we may also want to verify that it's a protocol we support or can switch to. For example, gopher:// would be invalid, but we could switch feed:// to http://

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a cool idea. I won't do it in this particular spot (resemblesUrl) because it might be reused by other things, but worth us keeping in mind.

} );

it( 'should return false if the string is not a URL', () => {
const source = 'exampledotcom';
Copy link
Contributor

@blowery blowery May 8, 2017

Choose a reason for hiding this comment

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

other test cases:
..com
. . .com
...com
.
<unicode non breaking spaces>.com

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some extras in e1aa849 👍

@samouri
Copy link
Contributor

samouri commented May 8, 2017

there something funky going on with the generated url. Sample action dispatched:

{
  type: 'READER_RECORD_FOLLOW',
  payload: {
    url: 'undefined://google.com
  }
}

@bluefuton
Copy link
Contributor Author

there something funky going on with the generated url

Ah - addSchemeIfMissing needs the scheme provided, so it works slightly differently to the function it replaced. I'll fix 👍

@bluefuton bluefuton 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 May 8, 2017
@bluefuton bluefuton added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 8, 2017
@jancavan
Copy link
Contributor

jancavan commented May 8, 2017

This happens in <660px. Fixed in: 86f14aa

screenshot 2017-05-08 13 24 09

@bluefuton It doesn't follow the site when I hit enter?
Nvm, just read this 😄

keyboard shortcut for return/enter/submit

@samouri
Copy link
Contributor

samouri commented May 8, 2017

giving this another test and if all looks good I'll 🚢 .

@samouri
Copy link
Contributor

samouri commented May 8, 2017

testing checked out.

@@ -94,7 +97,7 @@ class FollowingManage extends Component {
this.resizeSearchBox();

// this is a total hack. In React-Virtualized you need to tell a WindowScroller when the things
// above it has moved with a call to updatePosision(). Our issue is we don't have a good moment
// above it has moved with a call to updatePosition(). Our issue is we don't have a good moment
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't notice this before. thanks for the typo fix :)

@samouri samouri added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 8, 2017
@samouri samouri merged commit 23486db into master May 8, 2017
@samouri samouri deleted the add/reader/following-manage-search-by-url branch May 8, 2017 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Reader The reader site on Calypso. [Size] L Large sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants