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

Feature/54464 snappier progress popover #15383

Merged
merged 6 commits into from
Apr 25, 2024

Conversation

ulferts
Copy link
Contributor

@ulferts ulferts commented Apr 25, 2024

TODO

  • reduced the debounce time to 100ms
  • applied data-turbo-permanent attribute on focused field in the progress popover and prevented a DOM element with such an attribute from being updated. This prevents inputs from being eaten up.
    Using an attribute is in line with how Turbo does it so once the Turbo-bug on js triggered frame updates is fixed, it should still work out of the box.

    That path was abandoned again as idiomorph has a flag to be passed in when morphing ignoreActiveValue which, while documentation is sparse, seems to provide the behaviour we want out of the box.
  • removed the duplicate event listener on turbo:before-frame-render from setup.ts. It is only necessary for js triggered updates in the preview controller and not on a general level.
  •  replaced morphdom by idiomorph as that library is used by Turbo anyway.

https://community.openproject.org/wp/54464

@ulferts ulferts force-pushed the feature/54464-snappier-progress-popover branch from 7b62365 to c4ffc69 Compare April 25, 2024 13:39
@ulferts ulferts marked this pull request as ready for review April 25, 2024 13:53
// TODO: Ideally morphing in this single controller should not be necessary.
// Turbo supports morphing, by adding the <turbo-frame refresh="morph"> attribute.
// However, it has a bug, and it doesn't morphs when reloading the frame via javascript.
// See https://github.com/hotwired/turbo/issues/1161 . Once the issue is solved, we can remove
// this code and just use <turbo-frame refresh="morph"> instead.
this.frameMorphRenderer = (event:CustomEvent<TurboBeforeFrameRenderEventDetail>) => {
event.detail.render = (currentElement:HTMLElement, newElement:HTMLElement) => {
morphdom(currentElement, newElement, { childrenOnly: true });
Idiomorph.morph(currentElement, newElement, { ignoreActiveValue: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

Well well well... Seems they've thought of this already 😆

@aaron-contreras aaron-contreras self-requested a review April 25, 2024 14:43
Copy link
Contributor

@aaron-contreras aaron-contreras left a comment

Choose a reason for hiding this comment

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

Very nice @ulferts !

@aaron-contreras aaron-contreras merged commit b15d852 into dev Apr 25, 2024
12 checks passed
@aaron-contreras aaron-contreras deleted the feature/54464-snappier-progress-popover branch April 25, 2024 15:00
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