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

fix: Comments navigation issues. #2638

Merged
merged 16 commits into from Oct 28, 2021

Conversation

victorgcramos
Copy link
Member

@victorgcramos victorgcramos commented Oct 15, 2021

This PR fixes some comments navigation issues.

  • Removes duplicate comments and votes requests
  • Fixes comment area scroll
  • Fixes GoBack link and history past locations
  • Store comments filter on localstorage instead of url query
  • Fixes comments re-rendering caused by modal actions side-effects

Closes #2637
Closes #2633
Closes #2636
Closes #2620
Closes #2615

Copy link
Member

@amass01 amass01 left a comment

Choose a reason for hiding this comment

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

@victorgcramos Nice job so far, I know it's a wip
just pointed out some improvements to the
current state in inline comments.

src/containers/Comments/Comment/Comment.jsx Outdated Show resolved Hide resolved
src/components/Router/RouterProvider.jsx Outdated Show resolved Hide resolved
@victorgcramos
Copy link
Member Author

Screen Shot 2021-10-18 at 10 59 19 PM

@victorgcramos victorgcramos marked this pull request as ready for review October 19, 2021 02:01
Copy link
Member

@tiagoalvesdulce tiagoalvesdulce 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. Left a few suggestions.

src/hooks/api/useComments.js Outdated Show resolved Hide resolved
src/hooks/api/useComments.js Outdated Show resolved Hide resolved
Copy link
Member

@lukebp lukebp left a comment

Choose a reason for hiding this comment

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

  1. The record "Go Back" button is not taking you back to the home page, it's taking you back to the previous URL. If a user is clicking through comments, the back button will take them to the previous comment link that they were at.

  2. Can we clean this flickering up and smooth it out?

tmp-2021-10-22_16.19.33.mp4

@victorgcramos
Copy link
Member Author

@lukebp changed the Go Back link a bit. If location is records/:token, it redirects to home. If records/:token/comments/:commentid, it redirects to records/:token full details page.

The navigation hierarchy can be defined using the hierarchy prop on GoBackLink, it uses react-router match methods, so devs can define the navigation flow easily, by just passing the match strings array you want.


E2E Snapshots

Screen Shot 2021-10-25 at 7 37 01 PM

Copy link
Member

@lukebp lukebp left a comment

Choose a reason for hiding this comment

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

@victorgcramos that makes sense. Nice work.

tACK on Firefox.

Needs code approval from @tiagoalvesdulce.

Copy link
Member

@lukebp lukebp left a comment

Choose a reason for hiding this comment

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

I found one more potential issue related to comment navigation. Click an external link in a comment:

  1. There is unnecessary flickering caused by a rerender of the background when the link modal pops up. The expected behavior is to not see any background flickering.
  2. Nagivate to the link. The browser tab will change. Change the browser tab back to politeia and the view will have repositioned to the top of the comment section and also isn't displaying the page properly. The expected behavior is for the view to still be positioned at the same location from when you originally clicked the link.

The screen recording is from this proposal: https://proposals.decred.org/record/c1f5b5c

tmp-2021-10-27_08.17.20.mp4

@victorgcramos
Copy link
Member Author

@lukebp can you check if it happens on this PR? This is how it looks like:

2638-comments-external-links

Copy link
Member

@lukebp lukebp left a comment

Choose a reason for hiding this comment

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

@victorgcramos must already be fixed. Thanks for verifying.

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants