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

feat(webapps): add login plugin point #3461

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Conversation

tasso94
Copy link
Member

@tasso94 tasso94 commented May 26, 2023

related to #3412

@tasso94 tasso94 added the ci:e2e Runs the frontend end-to-end tests. label May 26, 2023
@tasso94 tasso94 self-assigned this May 26, 2023
@tasso94 tasso94 added the ci:webapp-integration Runs the webapp integration builds. label Jun 1, 2023
@tasso94 tasso94 force-pushed the 3412-login-page-plugin-point branch from 148ceaf to cb92f89 Compare June 5, 2023 09:45
@tasso94 tasso94 force-pushed the 3412-login-page-plugin-point branch from cb92f89 to 34a37d5 Compare July 4, 2023 13:57
@tasso94 tasso94 requested a review from danielkelemen July 4, 2023 14:01
Copy link
Member

@danielkelemen danielkelemen 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 to me.

  • 👍 I like the simplicity. Also the example and how it demonstrates a simple use-case.
  • 💬 It would be nice if based on the result the plugins could be re-rendered instead of hiding and displaying elements from the data plugin. What do you think? I feel like it's better if the plugin controls itself.

@tasso94
Copy link
Member Author

tasso94 commented Jul 18, 2023

💬 It would be nice if based on the result the plugins could be re-rendered instead of hiding and displaying elements from the data plugin. What do you think? I feel like it's better if the plugin controls itself.

Thanks for raising this.

I think that the solution, while sounding attractive at first glance, has disadvantages compared to the approach I've chosen:

  • Data plugin points can be used for all other render plugin points and are not strictly bound to just one.
  • When initially loading the render plugin point, we would have to synchronize it with the HTTP request, which leads to delays and may only be necessary in some cases (not for all use cases).
  • Unconditional re-rendering bound to the HTTP request is inefficient and might not be desired for every use case.
  • When a render plugin needs to consume data from multiple (possibly unrelated) sources, the result of all requests needs to be awaited before the plugin point can be rendered.

@tasso94 tasso94 merged commit 87e52cd into master Jul 18, 2023
@tasso94 tasso94 deleted the 3412-login-page-plugin-point branch July 18, 2023 13:24
mboskamp pushed a commit that referenced this pull request Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:e2e Runs the frontend end-to-end tests. ci:webapp-integration Runs the webapp integration builds.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants