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

DRAFT: HRN 598 (1): Scroll to Post Comment Path #653

Draft
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

thomasgwatson
Copy link
Contributor

closes #598

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (671fa90) 47.53% compared to head (7959059) 48.39%.

❗ Current head 7959059 differs from pull request most recent head 93d23ae. Consider uploading reports for the commit 93d23ae to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #653      +/-   ##
==========================================
+ Coverage   47.53%   48.39%   +0.86%     
==========================================
  Files         424      411      -13     
  Lines        7304     7083     -221     
  Branches     2024     1947      -77     
==========================================
- Hits         3472     3428      -44     
+ Misses       3832     3655     -177     
Files Coverage Δ
...reens/NotificationsList/NotificationsList.store.js 100.00% <100.00%> (ø)
src/screens/PostDetails/PostDetails.js 78.37% <ø> (ø)
src/store/models/Notification.js 35.41% <0.00%> (ø)
src/util/navigation.js 9.18% <33.33%> (ø)
src/components/Comment/Comment.js 0.00% <0.00%> (ø)
src/components/Comments/Comments.js 0.00% <0.00%> (ø)

... and 39 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@lorenjohnson lorenjohnson left a comment

Choose a reason for hiding this comment

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

Please consider my comment here on the PR and the one at #598 (comment)

@@ -18,6 +18,7 @@ import ImageAttachments from 'components/ImageAttachments'
export default function Comment ({
comment,
clearHighlighted,
commentIdFromParams,
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to keep this component dumb in-line with what is done with highlighted. I was hoping for this feature we would re-use the highlighted and clearHighlight functionality I added to the component and that it'd be controlled above where those are already managed. In particular setting highlighted on this comment when navigating to the screen via posts/:postId/comments/:commentId, and clearHighlight after a timer of 1-5 seconds. I find the current border styling, and it sticking around forever after navigating a little distracting. The QA part of that all IMHO of course, but as for the code review please consider keeping this component dumb and doing display things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spent the morning trying to revise based on this and got #656

I'm guessing that there is some weird race-condition that isn't making the scroll reliable.

@lorenjohnson
Copy link
Member

Also see comment here: #598 (comment)

@thomasgwatson
Copy link
Contributor Author

thomasgwatson commented Sep 4, 2023 via email

@lorenjohnson lorenjohnson changed the title Hrn 598 scroll to comment in path HRN 598: Scroll to Post Comment Path Sep 7, 2023
@lorenjohnson lorenjohnson changed the title HRN 598: Scroll to Post Comment Path DRAFT HRN 598: Scroll to Post Comment Path Sep 9, 2023
@lorenjohnson lorenjohnson changed the title DRAFT HRN 598: Scroll to Post Comment Path (DRAFT) HRN 598: Scroll to Post Comment Path Sep 9, 2023
@lorenjohnson lorenjohnson changed the title (DRAFT) HRN 598: Scroll to Post Comment Path (DRAFT 1) HRN 598: Scroll to Post Comment Path Sep 9, 2023
@lorenjohnson lorenjohnson changed the title (DRAFT 1) HRN 598: Scroll to Post Comment Path DRAFT: HRN 598 (1): Scroll to Post Comment Path Jun 19, 2024
@lorenjohnson lorenjohnson marked this pull request as draft October 3, 2024 10:59
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.

Clicking on comment notifications scrolls to that comment
2 participants