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

Blog index with tags rebase #3464

Merged
merged 12 commits into from
Aug 19, 2019
Merged

Blog index with tags rebase #3464

merged 12 commits into from
Aug 19, 2019

Conversation

Pomax
Copy link
Contributor

@Pomax Pomax commented Jul 29, 2019

Closes #3371
Related PRs/issues #3427 (#3370)

This adds tag filtering to any page of type IndexPage - e.g. if there is a listing page with slug /blog, visiting /blog/tags/tag1 or /blog/tags/tag1/tag2/tag3/... effects listing only pages posts that match that specific tag, or any of the specified tags, respectively.

(tags as used in urls should be the tag slug: Internet Health becomes internet-health, etc.)

Test URL

https://foundation-mofostaging-pr-3464.herokuapp.com/en/blog/

Also click on any of the blog cards' main tag to go to the dedicated tag page, e.g:

https://foundation-mofostaging-pr-3464.herokuapp.com/en/blog/tags/iot

Also also, verify that "related posts" blog cards still work:

https://foundation-mofostaging-pr-3464.herokuapp.com/en/blog/initial-test-blog-post-with-fixed-title/

Deviations from the design

As tags are no longer "filters" but real URLs, so the "clear filters" link has been (for now) omitted, in favour of people using their browser's "back" button.

The "result" phrasing has been changed to "X pages for (tag)", as the URL and page content does not suggest any searching/filtering happens, so talking about "results" without the user performing a search or active filter was problematic.

On any tag-specific page, blog cards show all tags, not just their "main" tag, or just the tag that page is for.

Design questions

  • which styling is suppose to be used for the "pages found" line? Using body makes it feel very small.
  • should cards show all tags when specifically searching for a tag? If not, things like https://foundation-mofostaging-pr-3464.herokuapp.com/en/blog/tags/iot/ get confusion, as the page is supposedly for iot posts, but because that's not the main tag for some pages, it'll look like there are pages listed that don't belong on that page.

@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3464 July 29, 2019 16:47 Inactive
@Pomax Pomax requested review from mmmavis and patjouk July 29, 2019 16:48
@Pomax
Copy link
Contributor Author

Pomax commented Jul 29, 2019

adding @patjouk for review because I might have done something really silly, python wise (and it'll be good to know if I have! =D)

@Pomax Pomax mentioned this pull request Jul 29, 2019
@Pomax Pomax requested review from alanmoo and removed request for mmmavis July 29, 2019 19:41
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 July 30, 2019 16:58 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 July 31, 2019 16:03 Inactive
@Pomax Pomax requested a review from mmmavis July 31, 2019 16:03
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 July 31, 2019 17:40 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 August 1, 2019 15:04 Inactive
@Pomax Pomax requested review from patjouk and removed request for patjouk August 1, 2019 15:06
@Pomax
Copy link
Contributor Author

Pomax commented Aug 1, 2019

@alanmoo @mmmavis could this please get a review (@patjouk, I added you mostly for your expertise, if you feel this would cut into your work too much, feel free to remove yourself as reviewer)?

It's been sitting for a few days and is holding up other work around the same model =(

@alanmoo alanmoo temporarily deployed to foundation-mofostaging-pr-3464 August 1, 2019 18:11 Inactive
@Pomax Pomax requested a review from mmmavis August 1, 2019 18:20
Copy link
Contributor

@alanmoo alanmoo left a comment

Choose a reason for hiding this comment

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

Code looks good, just a couple questions. Thanks for so many comments!

Not approving yet because I want to try it out and think rebuilding the review app is the fastest way?

@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3464 August 1, 2019 18:26 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 August 1, 2019 18:40 Inactive
@Pomax Pomax added the blocked label Aug 1, 2019
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 August 1, 2019 19:11 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 August 1, 2019 22:02 Inactive
@Pomax Pomax removed the blocked label Aug 1, 2019
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 August 1, 2019 22:06 Inactive
@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3464 August 1, 2019 22:08 Inactive
@patjouk patjouk removed their request for review August 2, 2019 14:35
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 August 8, 2019 21:56 Inactive
@Pomax
Copy link
Contributor Author

Pomax commented Aug 8, 2019

@mmmavis @alanmoo one more pass?

Copy link
Contributor

@alanmoo alanmoo left a comment

Choose a reason for hiding this comment

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

The code looks fine, but I'm not convinced the term list is quite right. I see @mmmavis already had some comments about this, but the vertical alignment of the smaller text isn't sitting with me the right way.

My instinct is to keep the text the same size, border-radius back to 1em, remove the vertical-align property, and set the line-height of .term to 1.

Though the point could also be raised that this whole "pill" style feels out of place and not like anything else we do, which is why this whole thing feels weird to work on.

That all being said, this isn't significant enough to block landing this, but I would want Kristina to have another look at this when she gets back, as I think any adjustments here would be a matter of a designer and dev sitting together for 10 minutes.

@Pomax
Copy link
Contributor Author

Pomax commented Aug 9, 2019

No question, when Kristina's back we need to look at a lot of things.

@alanmoo
Copy link
Contributor

alanmoo commented Aug 9, 2019

If you're going to merge, file a bug for her.

Copy link
Collaborator

@mmmavis mmmavis left a comment

Choose a reason for hiding this comment

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

Yea same thoughts as Alan's. We will need to work with Kristina when she's back to fine tune stuff.

@mmmavis
Copy link
Collaborator

mmmavis commented Aug 9, 2019

Note Pulse also uses pill button so we should standardize the styling.

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 August 12, 2019 18:01 Inactive
@Pomax
Copy link
Contributor Author

Pomax commented Aug 12, 2019

I will leave this open until Kristina's back, since that's only a few days from now.

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 August 15, 2019 23:22 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 August 15, 2019 23:35 Inactive
@Pomax Pomax requested a review from kristinashu August 15, 2019 23:35
@Pomax
Copy link
Contributor Author

Pomax commented Aug 15, 2019

@kristinashu I've added in the tweaks from our call, test URLs are:

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3464 August 15, 2019 23:37 Inactive
Copy link

@kristinashu kristinashu left a comment

Choose a reason for hiding this comment

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

This is looking and working swimmingly!

@gideonthomas gideonthomas modified the milestones: Aug 5, Aug 19 Aug 19, 2019
@Pomax Pomax merged commit e7f2574 into master Aug 19, 2019
@Pomax Pomax deleted the blog-index-with-tags-rebase branch August 19, 2019 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page for /blog?tag=[name]
6 participants