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

DXCDT-392: Restore the storybook preview functionality on the universal login templates update #666

Merged
merged 4 commits into from
Mar 10, 2023

Conversation

sergiught
Copy link
Contributor

@sergiught sergiught commented Mar 10, 2023

🔧 Changes

This PR restored the Storybook preview functionality we had before when updating universal login branding templates. The feature was originally introduced through https://github.com/auth0/auth0-cli/pull/267/files#diff-1a3673a1bdb76086ba81986c7b6038734bd1a823766de0484f59e85bc6bc14a1, most of the core logic has been kept, however applied on a slightly modified package structure.

📚 References

🔬 Testing

📝 Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

@sergiught sergiught force-pushed the DXCDT-392/restore-storybook branch from 7853cef to d224f2f Compare March 10, 2023 14:15
Comment on lines +127 to +144
To change the text editor used for editing templates, rules, and actions, set the environment variable `EDITOR` to your
preferred editor. If choosing a non-terminal editor, ensure that the command starts the editor and waits for the files
to be closed before returning.

Examples:

```shell
export EDITOR="code -w"
# or
# Uses vscode with the --wait flag.
export EDITOR="code --wait"

# Uses sublime text with the --wait flag.
export EDITOR="subl --wait"

# Uses nano, a terminal based editor.
export EDITOR="nano"

# Uses vim, a terminal based editor.
export EDITOR="vim"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is vital that a non terminal editor is opened through the --wait (or similar) flag to ensure that the preview mechanism on the auth0 ul templates update works as expected.

Short: "Update the custom template for Universal Login",
Long: "Update the custom template for the New Universal Login Experience.",
Example: ` auth0 universal-login templates update
auth0 ul templates update`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed support for piping the template as an input to the command as that would skip the preview mechanism due to the "shouldPrompt" logic we have and it would just make things more confusing.

@sergiught sergiught force-pushed the DXCDT-392/restore-storybook branch from d224f2f to 457c019 Compare March 10, 2023 14:36
@sergiught sergiught marked this pull request as ready for review March 10, 2023 14:36
@sergiught sergiught requested a review from a team as a code owner March 10, 2023 14:36
github.com/getsentry/sentry-go v0.19.0
github.com/golang/mock v1.6.0
github.com/google/go-cmp v0.5.9
github.com/google/uuid v1.3.0
github.com/guiguan/caster v0.0.0-20191104051807-3736c4464f38
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rely on a dependency that has not been updated since Nov 2019? https://github.com/guiguan/caster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally not, but swapping this with something else was outside the scope of the restoration efforts. I can look into this a bit if we have some extra time before GA to remove the dependency.

Comment on lines 50 to 51
<li><a href="https://company.com/privacy">Privacy Policy</a></li>
<li><a href="https://company.com/terms">Terms of Service</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<li><a href="https://company.com/privacy">Privacy Policy</a></li>
<li><a href="https://company.com/terms">Terms of Service</a></li>
<li><a href="https://example.com/privacy">Privacy Policy</a></li>
<li><a href="https://example.com/terms">Terms of Service</a></li>

Let's use the IANA reserved domains for those.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

If we can improve over a docs snippet, why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree with you 👍🏻 I was just a bit wary of going off track on what we advertise within the docs themselves. I'll update this and the other snippets as well then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{%- auth0:head -%}
<style>
body {
background-image: url("https://images.unsplash.com/photo-1592450865877-e3a318ec3522?ixlib=rb-1.2.1&auto=format&fit=crop&w=2255&q=80");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use something a little more generic as a base template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any examples in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh super 💯 , thanks let me put one of those in action 💪🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Can the background image extend all the way under the login box, instead of having a solid color? And can the login box be horizontally centered as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, however I believe it's beneficial to showcase some custom logic within these sample templates, to ensure customers are aware that complex customizations are possible. To that extent I also added a comment to showcase how to use the script tag for the preview, as mentioned here: https://community.auth0.com/t/the-script-tag-breaks-the-auth0-cli-storybook/90526

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice pic, BTW.

@@ -0,0 +1,135 @@
<!doctype html><html lang="en"><head><meta charset="utf-8"><title>Storybook</title><meta name="viewport" content="width=device-width,initial-scale=1"><base target="_parent"><style>:not(.sb-show-main) > .sb-main,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<!doctype html><html lang="en"><head><meta charset="utf-8"><title>Storybook</title><meta name="viewport" content="width=device-width,initial-scale=1"><base target="_parent"><style>:not(.sb-show-main) > .sb-main,
<!doctype html><html lang="en"><head><meta charset="utf-8"><title>Universal Login Template</title><meta name="viewport" content="width=device-width,initial-scale=1"><base target="_parent"><style>:not(.sb-show-main) > .sb-main,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if err := ansi.Waiting(func() (err error) {
currentTemplate, err = cli.api.Branding.UniversalLogin()
if err != nil {
if mErr, ok := err.(management.Error); ok && mErr.Status() == http.StatusNotFound {
Copy link
Contributor

Choose a reason for hiding this comment

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

Eventually we could extract this logic somewhere, as this pattern of checking for error 404 (Management API quirk) has popped up repeatedly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, definitely a good point. There's much to be improved in terms of refactoring and project structure still.

Comment on lines 117 to 123
cli.renderer.Heading("universal login template")

if currentTemplate == nil {
cli.renderer.Infof("No custom template found. To set one, run: `auth0 universal-login templates update`.")
}

fmt.Println(currentTemplate.GetBody())
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have this in a Renderer func, inside the display package as usual?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually already do cli.renderer.Output(currentTemplate.GetBody()) this was just moved but I can update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant in a separate file inside the display package, as in:

Screenshot 2023-03-10 at 14 30 26

But it's a nit anyway, nevermind.

Use: "update",
Args: cobra.NoArgs,
Short: "Update the custom template for Universal Login",
Long: "Update the custom template for the New Universal Login Experience.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Long: "Update the custom template for the New Universal Login Experience.",
Long: "Update the custom template for the New Universal Login experience.",

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 is usually capitalized within our docs, e.g. https://auth0.com/docs/api/management/v2#!/Branding/put_universal_login.

@sergiught sergiught requested a review from Widcket March 10, 2023 16:58
@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2023

Codecov Report

Patch coverage: 7.09% and project coverage change: -0.81 ⚠️

Comparison is base (855584d) 51.28% compared to head (aa9d840) 50.48%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #666      +/-   ##
==========================================
- Coverage   51.28%   50.48%   -0.81%     
==========================================
  Files          92       93       +1     
  Lines       11576    11758     +182     
==========================================
- Hits         5937     5936       -1     
- Misses       5211     5394     +183     
  Partials      428      428              
Impacted Files Coverage Δ
internal/cli/universal_login.go 53.75% <ø> (+12.59%) ⬆️
internal/cli/universal_login_templates.go 7.09% <7.09%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines +300 to +302
cli.renderer.Infof(
"%s Once you close the editor, you'll be prompted to save your changes. To cancel, press CTRL+C.",
ansi.Faint("Hint:"),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should eventually add a renderer method for this.

Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

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

Nice work! I'd suggest replacing that unmaintained new dependency ASAP. Everything else is a non-blocking nit.

@sergiught
Copy link
Contributor Author

Thanks @Widcket 🙇🏻 , I'm gonna follow up on that dep first thing next week.

@sergiught sergiught merged commit dca152f into main Mar 10, 2023
@sergiught sergiught deleted the DXCDT-392/restore-storybook branch March 10, 2023 18:08
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.

4 participants