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

[59769] When adding new relations, auto-scroll to show the newly added relation #17355

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

bsatarnejad
Copy link
Contributor

@bsatarnejad bsatarnejad commented Dec 4, 2024

Ticket

https://community.openproject.org/projects/stream-planning-and-reporting/work_packages/59769/activity

Screen recording

Screen.Recording.2024-12-04.at.16.44.53.mov

What are you trying to accomplish?

Scroll to the currently created relation in relations tab.

What approach did you choose and why?

After creating a relation, pass the relation-wp-id to the component for initializing it.

Merge checklist

  • Added/updated tests
  • Added/updated documentation in Lookbook (patterns, previews, etc)
  • Tested major browsers (Chrome, Firefox, Edge, ...)

@bsatarnejad bsatarnejad force-pushed the 59769-when-adding-new-relations-auto-scroll-to-show-the-newly-added-relation branch from 8f2522a to 6c71aed Compare December 5, 2024 08:54
@bsatarnejad bsatarnejad force-pushed the 59769-when-adding-new-relations-auto-scroll-to-show-the-newly-added-relation branch from 34e143b to 8775574 Compare December 5, 2024 13:12
@bsatarnejad bsatarnejad self-assigned this Dec 5, 2024
Copy link
Contributor

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

Please have a look at my remarks.


connect() {
// Wait to ensure DOM is fully loaded
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

setTimeout does not automatically ensure you that the DOM is fully loaded afaik, there might still be some delays that could prevent that could from being executed.

@bsatarnejad bsatarnejad requested a review from HDinger December 11, 2024 07:08
Copy link
Contributor

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

I have left some comments below

}

waitForRenderAndAct() {
const element = document.querySelector('[data-scroll-to="true"]');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend to use a stimulus target instead of a normal data attribute. The value is crutial for this controller to work, so it should be an explicit target imho.

if (element) {
element.scrollIntoView({ behavior: 'smooth', block: 'center' });
} else {
requestAnimationFrame(this.waitForRenderAndAct.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

This creates like an infinte loop when there is no scroll target (e.g. on a fresh page reload). The code is just executed endlessly.
Further, requestAnimationFrame is just meant for animations and will execute the code 60 times per second (assuming 60hertz screen), because that makes sense for animations. But you don't have an animation here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants