Accept comments argument in showSelectedPost #11606
Conversation
Currently when clicking on a comment link in a stream we are passing a `comments` attribute to `showSelectedPost` (via an object), however we aren’t destructuring it and are doing nothing with it. This commit adds `comments` to the arguments destructuring in `showSelectedPost` and then passes the argument down to `showFullPost`, which already supports and is smart enough to add `#comments` to the URL. As a result, now clicking on a the comments link in any card takes us to the comments section of the post, not the title. There are some limited tests, too. Fixes #10951.
nb
added
[Feature] Reader
|
|
||
| it( 'redirects if passed a post key', () => { | ||
| showSelectedPost( { postKey: { feedId: 1, postId: 5 } } ); | ||
| expect( pageSpy ).to.have.been.calledOnce; |
There was a problem hiding this comment.
may be good to verify that the url does not have "#comments" here
There was a problem hiding this comment.
Agreed. I wanted to hear your thoughts on refactoring this a bit before adding a lot more tests. This suite is missing many other branches of the code.
|
Thank you @nb for addressing this! Code looks great. I love the tests :) |
|
On your design notes: |
|
Functionality is still broken for cards in the SearchStream. I'll plop in a commit with a fix |
samouri
added
[Status] Needs Author Reply
and removed
[Status] Needs Review
samouri
added
[Status] Needs Review
matticbot
removed
the
[Status] Needs Review
|
@blowery wouldn’t it have been better if we had squashed the commits in this case? The search stream change doesn’t seem logically separate from the first one. |
|
@nb I tend to avoid squashing when there are multiple authors |
|
helps to preserve blame info down the road |
Fixes #10951 – when clicking the Comments link on a card in the reader it doesn’t take us to the comments section of a post.
Implementation details are in the commit message.
A couple of design notes:
pagethis deep the call chain was unexpected for me. It would’ve felt a lot more natural ifpagecalls were limited to actions/controllers, or at least to components, not library functions.