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

Replace dynamic bootstrapped components with angular elements #16431

Merged
merged 12 commits into from
Aug 19, 2024

Conversation

oliverguenther
Copy link
Member

@oliverguenther oliverguenther commented Aug 13, 2024

To pave the way towards turbo, we will need to allow dynamic bootstrapping of angular elements on these visits. As we've made good experiences with Angular Elements in the past, this PR tries to get rid of all dynamically bootstrapped components in favor of angular elements.

There is one case where this is not possible: the col.opHighlight is used for table highlighting of columns, which is an attribute directive an cannot be replaced easily. This PRs replaces it with a stimulus controller instead.

https://community.openproject.org/work_packages/57269

@oliverguenther oliverguenther force-pushed the chore/remove-dynamic-bootstrapping branch 12 times, most recently from 735ba00 to 8f4219c Compare August 15, 2024 11:11
@oliverguenther oliverguenther marked this pull request as ready for review August 15, 2024 11:14
@oliverguenther oliverguenther requested a review from HDinger August 15, 2024 11:14
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.

Looks good as far as I can tell. I found some issues with the highlighting stimulus controller:

  • On all WorkPackage tables the columsn are not highlighted
Bildschirmfoto 2024-08-16 um 07 57 36
  • With the current approach, we highlight all columns and not only those that we previously marked with the opHighlightCol directive. That results in columns with empty header being highlighted as well. However, that might not be such an issue. We could ask product about that.
Bildschirmfoto 2024-08-16 um 07 53 50
  • On the project list, there is some weird effect when hovering the hierarchy column as well as the last coumn with the "more" actions. I assume, that this closely related to the point above, as those columns were not meant to be hovered.
Bildschirmfoto 2024-08-16 um 08 01 37

@oliverguenther oliverguenther force-pushed the chore/remove-dynamic-bootstrapping branch from 8f4219c to 34ca056 Compare August 16, 2024 06:58
@oliverguenther
Copy link
Member Author

@HDinger I knew this would come back to bite me. I now provided a way to disable the highlighting on a per col level and restored all the previous places

@oliverguenther oliverguenther force-pushed the chore/remove-dynamic-bootstrapping branch from 34ca056 to deef20e Compare August 16, 2024 16:19
@oliverguenther oliverguenther force-pushed the chore/remove-dynamic-bootstrapping branch 2 times, most recently from 9652dce to aaff7b2 Compare August 19, 2024 05:01
@oliverguenther oliverguenther requested a review from HDinger August 19, 2024 05:28
@oliverguenther oliverguenther force-pushed the chore/remove-dynamic-bootstrapping branch from aaff7b2 to 47da171 Compare August 19, 2024 05:34
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.

LGTM 👍

There is one special behavior on the project list, where the last column shows a hover effect although there is no header. However, since that is the only table where I saw that, I am fine with extracting that into a separate ticket.

Bildschirmfoto 2024-08-19 um 14 19 17

@HDinger HDinger merged commit bd755d4 into dev Aug 19, 2024
12 checks passed
@HDinger HDinger deleted the chore/remove-dynamic-bootstrapping branch August 19, 2024 12:38
@HDinger
Copy link
Contributor

HDinger commented Aug 19, 2024

LGTM 👍

There is one special behavior on the project list, where the last column shows a hover effect although there is no header. However, since that is the only table where I saw that, I am fine with extracting that into a separate ticket.
Bildschirmfoto 2024-08-19 um 14 19 17

I created a ticket for that: https://community.openproject.org/projects/openproject/work_packages/57329/activity

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