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

Decouple discussion-rendering from DCR #8243

Closed
bryophyta opened this issue Jun 6, 2022 · 2 comments
Closed

Decouple discussion-rendering from DCR #8243

bryophyta opened this issue Jun 6, 2022 · 2 comments

Comments

@bryophyta
Copy link
Contributor

There are currently a few issues with the discussion rendering on the dotcom site (e.g. DCR #4836. To address these issues effectively it seems like we need to reduce the coupling between DCR and discussion-rendering.

As far as I understand it, the underlying requirements which have led to the current challenges are as follows:

  • 'Scroll to comment' functionality may require loading a comment which is not in the DOM at the time a user clicks 'jump to comment' (or when they navigate to the page using a permalink with the comment's id in the URL). This means that we need to handle the scrolling in javascript.
  • If the targeted comment isn't loaded when the user clicks 'scroll to comment', the comment needs to be fetched from the context endpoint of the discussion API, which requires both the comment's id and also the current state of the filters, pagination, etc. in order to return the comment as part of the right page.

In the current setup, discussion-rendering owns the state which says which page, filters, etc. have been selected. And DCR makes the request to the discussion context API. (It gets the state from d-r via localstorage.)

It seems preferable to locate as much of the state and business logic as possible in one or the other of these repos, rather than having these interdependent bits of functionality spread across two codebases (and two React components). Initially we'd discussed lifting the state up to DCR, but currently the thinking seems to be more towards centralising the functionality in d-r and passing down data where needed as props.

Given this, it seems like there are at least two issues remaining:

  1. Does this mean adding functionality into d-r that DCR needs but that future users of the package (e.g. Apps rendering) don't need/want? (e.g. interacting with the URL, manipulating scroll position). If so, can we mitigate these and/or make them optional?
  2. This refactor will not, in itself, fix issues like the 'jump to comment' bug (though it should make it easier to create a long-term fix for them). We still need to add e.g. some way of keeping track of scroll attempts, to avoid jumping back to the same comment when it's not intended.
@mxdvl
Copy link
Contributor

mxdvl commented Jan 11, 2024

This will be addressed by #10023

@alinaboghiu
Copy link
Member

Closing in favour of #10023

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

No branches or pull requests

3 participants