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

Render app credential warnings with BigPipe #957

Draft
wants to merge 3 commits into
base: 3.x
Choose a base branch
from

Conversation

mxr576
Copy link
Contributor

@mxr576 mxr576 commented Oct 11, 2023

This solves Problem 1 from #944.

@mxr576
Copy link
Contributor Author

mxr576 commented Oct 11, 2023

So this is the first version of the solution and it is subject to changes, especially around wording.

The main idea that we would keep the old way until 4.0.0 because downstream projects may (and most probably) customized how app credentials are rendered.

Also, if you would wonder, this also works without Big Pipe having enabled, it just renders everything synchronously, so we do not need to make it a requirement - maybe a warning on admin/reports/status page for that would be useful if it is not enabled.

@@ -138,6 +139,12 @@ function apigee_edge_theme() {
'render element' => 'elements',
'base hook' => 'apigee_secret',
],
'apigee_app_credential_warnings' => [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should superior flexibility for downstream projects to customize these.

@@ -0,0 +1,5 @@
<div class="app-credential-warnings">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO add docblocks

@@ -0,0 +1,5 @@
<div class="app-credential-warnings">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO add docblocks

@mxr576 mxr576 force-pushed the app_credential_warnings_bigpipe branch from 4ee80ed to a6c1b79 Compare October 11, 2023 14:20
* {@inheritdoc}
*/
public static function trustedCallbacks(): array {
return ['lazyBuilder'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Why on earth invokable classes are not supported...???!)

@mxr576
Copy link
Contributor Author

mxr576 commented Nov 7, 2023

@divya-intelli @giteshk what is your opinion about the suggested approach? Does this PR worth to be continued? Anything that you would change on the base idea?

@@ -66,6 +66,9 @@ apigee_edge.common_app_settings:
callback_url_placeholder:
type: label
label: 'Placeholder for a Callback URL'
app_credential_warnings_bc_mode:
type: boolean
deprecated: "The 'app_credential_warnings_bc_mode' config schema is deprecated in Apigee Edge 3.0.3 and will be removed from Apigee Edge 4.0.0."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Todo adjust version number before merge.

@divya-intelli
Copy link
Collaborator

@divya-intelli @giteshk what is your opinion about the suggested approach? Does this PR worth to be continued? Anything that you would change on the base idea?

@mxr576 , its good to have this feature which will speed up the page.
Thank you and appreciate for your time.
We will review once the PR is ready.

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

Successfully merging this pull request may close these issues.

2 participants