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

Issue/3985 reader tag display name #3995

Merged
merged 22 commits into from
May 13, 2016
Merged

Conversation

nbradbury
Copy link
Contributor

@nbradbury nbradbury commented Apr 21, 2016

Resolves #3985 - Changes the reader to show tags using their display_name rather than their title. This is being done for consistency with Calypso, and the iOS app will also be making this change.

This required making quite a few changes to the reader code to ensure that tags are displayed - and added - the same as they are on the web. It also required updating the list of followed tags when a new one is added, to ensure local tags are formatted the same as server.

@aerych Mind giving this a quick spin to ensure it's working correctly? I'll ask an Android dev to do a code review afterwards.

@nbradbury nbradbury added this to the 5.4 milestone Apr 21, 2016
@aerych
Copy link
Member

aerych commented Apr 22, 2016

Hiya @nbradbury. I took the branch for a test drive. Tags appeared as expected in most cases (more on that in a bit) and I was able to preview a tag, follow a previewed tag, add a tag, and unfollow a tag.
While testing I did notice a couple of things:

If Discover is already selected:

Before installing the branch I was signed in with a self-hosted blog (no Jetpack) and Discover was the selected topic. After installing the branch Discover was still the selected topic but it would not sync any posts:
screen shot 2016-04-22 at 9 42 09 am

Discover shows up correctly in the list
screen shot 2016-04-22 at 9 42 29 am

After I switched to a different topic then back Discover would load correctly.

wpcom missing defauts

After signing into wpcom I went back to the reader and checked the list. The first thing that struck me as odd was a user created list was the selected topic in the list. When I opened the list, I saw that the default topics (liked, followed, Discover) were missing. My other user created list was also missing.
screen shot 2016-04-22 at 9 50 22 am

If I refreshed the selected user created list it wouldn't load any content. However, after selecting a different topic, then selecting the user created list again, it loaded content from Discover:
screen shot 2016-04-22 at 9 50 59 am

@nbradbury
Copy link
Contributor Author

@aerych Thanks for taking this for a test drive and for finding those ridiculous bugs. Looks like both of those were caused by the same thing - not setting the slug correctly when one wasn't passed - which I just fixed in 7a8013a.

@aerych
Copy link
Member

aerych commented Apr 22, 2016

Retested and it looks great :) Thanks Nick. :shipit: from me!

@tonyr59h tonyr59h self-assigned this Apr 25, 2016
@kwonye kwonye self-assigned this May 12, 2016
@kwonye
Copy link
Contributor

kwonye commented May 12, 2016

@nbradbury Looks like this missed the 5.4 cutoff. Do you mind if I switch it to the 5.5 milestone?

@nbradbury nbradbury modified the milestones: 5.5, 5.4 May 12, 2016
@nbradbury
Copy link
Contributor Author

Do you mind if I switch it to the 5.5 milestone?

Not a problem - I just did that.

+ " tag_display_name TEXT COLLATE NOCASE,"
+ " tag_title TEXT COLLATE NOCASE,"
+ " tag_type INTEGER DEFAULT 0,"
+ " endpoint TEXT,"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we fix the spacing/alignment of these two execSQL please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we fix the spacing/alignment of these two execSQL

Fixed in 09ecb34.

@kwonye
Copy link
Contributor

kwonye commented May 12, 2016

I'm seeing a lot of magic strings. While understandable in the DB functions, I believe they should be constants elsewhere. Can you make them constants please? (especially in the parsing of the JSON objects in ReaderTagActions.java and ReaderUpdateService.java)

@nbradbury
Copy link
Contributor Author

Can you make them constants please? (especially in the parsing of the JSON objects in ReaderTagActions.java and ReaderUpdateService.java)

In commit f95d572 I changed the JSON key names to constants in those two classes. Were there other places you think need similar changes?

@kwonye
Copy link
Contributor

kwonye commented May 13, 2016

In commit f95d572 I changed the JSON key names to constants in those two classes. Were there other places you think need similar changes?

Nope, those were the only noticeable ones. Works great! :shipit:

@kwonye kwonye merged commit 5ad1781 into develop May 13, 2016
@kwonye kwonye deleted the issue/3985-reader-tag-display-name branch May 13, 2016 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants