Fixed Reading List on Mobile + Small Improvements #12755

nickytonline · 2021-02-20T04:26:12Z

Details

What type of PR is this? (check all applicable)

Description

Implements changes proposed in #11983. One notable change is that we only filter by one tag at a time now.

Before

Mar-09-2021.15-48-54.mp4

After

Mar-31-2021.19-34-29.mp4

Related Tickets & Documents

Closes #11983

QA Instructions, Screenshots, Recordings

The reading list section should still be able to archive and unarchive reading list items.

Archiving

Navigate to http://localhost:3000/
In the feed, click on the save button of an article in the feed to add it to the reading list.
Navigate to http://localhost:3000/readinglist
Click on the Archive button of the reading list item.
The archived item will disappear from the reading list.
A snackbar item at the bottom left of the screen should say "Archiving..." and then disappear after a second or two.

Unarchiving

Navigate to http://localhost:3000/
In the feed, click on the save button of an article in the feed to add it to the reading list.
Navigate to http://localhost:3000/readinglist
Click on the Archive button of the reading list item.
The unarchived item will disappear from the archived reading list.
A snackbar item at the bottom left of the screen should say "Archiving..." and then disappear after a second or two.
Navigate to http://localhost:3000/readinglist/archive
Click on the Unarchive button of the archived reading list item.
A snackbar item at the bottom left of the screen should say "Unarchiving..." and then disappear after a second or two.

Filtering

Filtering the Reading List

Navigate to http://localhost:3000/
In the feed, click on the save button of an article in the feed to add it to the reading list. Do this with 4 or 5 articles so the reading list is a small size.
Navigate to http://localhost:3000/readinglist
In the search text box, enter in some text that is in the title of an article. It will filter so only article titles with that text are visible.
In the filter by tag drop down, select a tag. It will filter so only article titles with the current filter text and articles with the selected tag are visible.
Clear the search text box. It will filter so only articles with the selected tag are visible.

Filtering the Archive

To test the same functionality in the archived view:

From the reading list with the existing list from the previous test set up, click on the archive button of an item in the reading list for as many articles as you'd like.
Repeat steps 4-6 from the "Filtering the Reading List" section of QA.

UI accessibility concerns?

I fixed the markup so that everything is contained within the <main /> landmark now. There is a colour contrast issue with the <select /> being reported, but this is not related to the work in the PR. This appears to be more of a global issue with <select />s. cc: @ludwiczakpawel

@aitchiss, the fieldset on the left when on the medium breakpoint and up is now a group of radio buttons since we only allow for filtering on one tag now. The keyboard navigation appears to be working well as is Voice Over, but please take it for a spin to see if there are any gaps in the work there.

Added tests?

Yes
No, and this is why: please replace this line with details on why tests
have not been included
I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

I've updated the Developer Docs and/or
Admin Guide, or
Storybook (for Crayons components)
I've updated the README or added inline documentation
I will share this change in a Changelog
or in a forem.dev post
I will share this change internally with the appropriate teams
I'm not sure how best to communicate this change and need help
This change does not need to be communicated, and this is why not: please
replace this line with details on why this change doesn't need to be
shared

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

nickytonline · 2021-02-20T04:28:06Z

I've scrapped some component tests that existed for this and will write some end to end tests instead.

ludwiczakpawel

I know it's a Draft PR but you added me as reviewers so here I am :D

Desktop UI regression. This one is IMO a blocker - afair we didn't want to replace sidebar nav with select field everywhere but only on smaller breakpoint(s). Bigger breakpoints should keep the left sidebar. This is similar to what we do on notifications and settings and that's likely the pattern we want to keep elsewhere.
Going back to main view - blocker as well. When I choose a tag I can no longer go back to "main" view of Reading list - without any tag selected. That should probably be its own option in the select like "Everything" or something..
a11y issue - blocker too. When I use keyboard to choose a tag in this select, then hit Shift + Tab to go to previous field (search field), the filtering is being reset:

Nagranie.z.ekranu.2021-02-22.o.09.24.50.mov

It's probably because search field, even if empty, is running a call and returns results matching the query (query is empty so it return everything). In other words, we should probably limit search field to only search through current view (selectedTag).

Not a blocker but something to think about in future is being able to reflect the current filter with URL. In other words, when you change the filtering we could update the browser's URL too to dev.to/readinglist/javascript. I'm aware it's something we don't have right now so there's no expectation to build it :D But you know, just something to keep in back of our heads.

app/javascript/readingList/components/TagList.jsx

ludwiczakpawel · 2021-02-22T08:14:30Z

app/javascript/readingList/readingList.jsx

 import { ItemListItem } from './components/ItemListItem';
 import { ItemListItemArchiveButton } from './components/ItemListItemArchiveButton';


We should at some point look into components names: ItemListItem is probably not very descriptive...

ludwiczakpawel · 2021-02-22T08:22:34Z

app/javascript/readingList/components/TagList.jsx

+      onBlur={(event) => {
+        // We need blur for a11y, but we also don't want to make the same search query twice
+        // if the tag hasn't changed since the previous onChange.
+        const { value } = event.target;
+
+        if (value === selectedTag) {
+          return;
+        }
+
+        onSelectTag(event);
+      }}


Can you help me understand what does it do? I couldn't notice any difference when I removed this part of code.

^ I bump this question.

nickytonline · 2021-02-22T11:31:47Z

@ludwiczakpawel, I must have misread the comment about select. I thought we wanted it for desktop as well.

I'll look at the other issues as well you mentioned.

In regards to keeping the filter in the URL, I completely agree, but that should be a separate PR a that changes how the entire state is managed for this component.

ludwiczakpawel · 2021-02-22T12:24:58Z

@nickytonline just to make sure I'm not making things up right now :D:

Expected behavior
...
2. Collapse nav into the select on mobile breakpoint.

^ that's also being mentioned in other comments in the same issue.

nickytonline · 2021-02-24T21:27:49Z

@ludwiczakpawel, @lisasy I've reworked this. The sidebar remains until we hit the smallest breakpoint. I took some liberties from some minor spacing changes, so let me know what you think.

@aitchiss, I also reworked the side bar to be a group of checkboxes even though visually it looks the same. It helps with the keyboard nav and voice over.

Here is the loom video, https://www.loom.com/share/c78e3427cfdf4b5ba01506ddd01ace32

Tests are still broken, but I will sort that out.

nickytonline · 2021-02-25T16:57:51Z

I'm putting this on hold after I'm back as I'm looking into a higher priority issue in tegards to service workers, but if you can take this for a test drive @lisasy, @ludwiczakpawel, and @aitchiss, it'd be much appreciated. The only blocker for this PR at the moment is tests.

nickytonline · 2021-02-26T03:27:42Z

As mentioned, I'll resume this when I'm back from my time off, but I put up some e2e tests @aitchiss. I discovered that Cypress is not evaluating window.matchMedia properly and I think it's related to cypress-io/cypress#311. The browser API is working, but it always returns false when trying to match breakpoints.

ludwiczakpawel · 2021-03-02T07:53:42Z

@nickytonline just turn it to "Ready for review" when ready :)

lisasy · 2021-03-08T22:46:17Z

I can't seem to review this PR on my local dev, so my feedback is based on the video in the description. On the smallest breakpoint, there ought to be appropriate margin around the heading, search input, dropdown, and button so that these components aren't touching the edges of the device.

nickytonline · 2021-03-08T23:00:01Z

I can't seem to review this PR on my local dev, so my feedback is based on the video in the description. On the smallest breakpoint, there ought to be appropriate margin around the heading, search input, dropdown, and button so that these components aren't touching the edges of the device.

That seems to be the way the crayon layout class works, but I can definitely add margin. Also, those videos are out of date as that was my initial pass. I'll be posting updated videos shortly and will pull this out of draft mode.

…fix-reading-list-view-on-mobile

aitchiss

Checked this locally on desktop and android mobile and all looking good - I just have some v minor change requests/suggestions 🙂

app/javascript/readingList/__tests__/readingList.test.jsx

app/javascript/readingList/components/TagList.jsx

app/javascript/readingList/readingList.jsx

ludwiczakpawel

NICE!

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

…fix-reading-list-view-on-mobile

nickytonline · 2021-04-08T11:36:00Z

I also tested this in iOS 14.4.2 and all looks good.

aitchiss

Nice! 🚀

pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Feb 20, 2021

nickytonline requested review from ludwiczakpawel and aitchiss February 20, 2021 04:26

nickytonline requested a review from lisasy February 20, 2021 04:44

nickytonline changed the title ~~Some div soup to semantic markup.~~ Fixed Reading List on Mobile + Small Improvements Feb 20, 2021

ludwiczakpawel suggested changes Feb 22, 2021

View reviewed changes

nickytonline mentioned this pull request Feb 24, 2021

Added the @components webpack alias to the project #12808

Merged

14 tasks

nickytonline added 14 commits March 8, 2021 18:03

Some div soup to semantic markup.

8f0d757

Small refactor to inline mapping of available tags.

2e3f26b

Renamed <ItemListTags /> component to <TagList />.

ae4f420

Now a select is used for picking a tag to filter on.

b44ef9b

Added custom Cypress command to create an article.

4afecb3

Added documentation for the create article custom command.

94d6cbc

Removed unnecesary properties from payload to create an article.

f195525

reading list mobile view wip.

77a65d0

Reworked styles in <TagList />.

f605ccf

Reworked reading list to use <MediaQuery /> component.

9be087b

Removed bottom padding from reading list header.

6f4aaa3

styling tweaks if there are no available tags.

c9baf29

Added some E2E tests.

4518ded

Removed reading list component test in favour of e2e test.

8526271

nickytonline added 3 commits April 7, 2021 10:35

Fixed empty state popping up between search queries.

88747f3

Merge remote-tracking branch 'origin/master' into nickytonline/11983-…

c17ea27

…fix-reading-list-view-on-mobile

Fixed casing of fixture filenames.

c9f18b6

aitchiss suggested changes Apr 8, 2021

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Apr 8, 2021

ludwiczakpawel approved these changes Apr 8, 2021

View reviewed changes

Update app/javascript/readingList/readingList.jsx

f9c222d

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Apr 8, 2021

nickytonline added 5 commits April 8, 2021 07:19

Merge remote-tracking branch 'origin/master' into nickytonline/11983-…

5dfd101

…fix-reading-list-view-on-mobile

Reverted change in reading list component test.

1ce9470

Added missing JSDoc comment.

85442a6

Now links are in an unordered list.

593240e

Promoted some CSS classes from the <nav /> to the <ul /> for spacing.

28e6518

aitchiss approved these changes Apr 8, 2021

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Apr 8, 2021

aitchiss merged commit 86deddb into master Apr 8, 2021

aitchiss deleted the nickytonline/11983-fix-reading-list-view-on-mobile branch April 8, 2021 11:53

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Apr 8, 2021

nickytonline mentioned this pull request Apr 8, 2021

[15 Min Fix] Now the load more button only appears (if necessary) when the reading list is finished loading #13327

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Reading List on Mobile + Small Improvements #12755

Fixed Reading List on Mobile + Small Improvements #12755

nickytonline commented Feb 20, 2021 •

edited

Loading

nickytonline commented Feb 20, 2021

ludwiczakpawel left a comment •

edited

Loading

ludwiczakpawel Feb 22, 2021

ludwiczakpawel Feb 22, 2021

ludwiczakpawel Mar 12, 2021

nickytonline commented Feb 22, 2021 •

edited

Loading

ludwiczakpawel commented Feb 22, 2021

nickytonline commented Feb 24, 2021

nickytonline commented Feb 25, 2021

nickytonline commented Feb 26, 2021

ludwiczakpawel commented Mar 2, 2021

lisasy commented Mar 8, 2021

nickytonline commented Mar 8, 2021

aitchiss left a comment

ludwiczakpawel left a comment

nickytonline commented Apr 8, 2021

aitchiss left a comment

		import { ItemListItem } from './components/ItemListItem';
		import { ItemListItemArchiveButton } from './components/ItemListItemArchiveButton';

Fixed Reading List on Mobile + Small Improvements #12755

Fixed Reading List on Mobile + Small Improvements #12755

Conversation

nickytonline commented Feb 20, 2021 • edited Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Archiving

Unarchiving

Filtering

Filtering the Reading List

Filtering the Archive

UI accessibility concerns?

Added tests?

[Forem core team only] How will this change be communicated?

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

nickytonline commented Feb 20, 2021

ludwiczakpawel left a comment • edited Loading

Choose a reason for hiding this comment

ludwiczakpawel Feb 22, 2021

Choose a reason for hiding this comment

ludwiczakpawel Feb 22, 2021

Choose a reason for hiding this comment

ludwiczakpawel Mar 12, 2021

Choose a reason for hiding this comment

nickytonline commented Feb 22, 2021 • edited Loading

ludwiczakpawel commented Feb 22, 2021

nickytonline commented Feb 24, 2021

nickytonline commented Feb 25, 2021

nickytonline commented Feb 26, 2021

ludwiczakpawel commented Mar 2, 2021

lisasy commented Mar 8, 2021

nickytonline commented Mar 8, 2021

aitchiss left a comment

Choose a reason for hiding this comment

ludwiczakpawel left a comment

Choose a reason for hiding this comment

nickytonline commented Apr 8, 2021

aitchiss left a comment

Choose a reason for hiding this comment

nickytonline commented Feb 20, 2021 •

edited

Loading

ludwiczakpawel left a comment •

edited

Loading

nickytonline commented Feb 22, 2021 •

edited

Loading