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.

Sign up for GitHub

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

Jump to bottom

Fix #1461 - Update UI of subdomain onboarding module #1513

Merged
merged 8 commits into from
Feb 9, 2022
Merged

Fix #1461 - Update UI of subdomain onboarding module #1513

merged 8 commits into from
Feb 9, 2022

Conversation

codemist
Copy link
Collaborator

@codemist codemist commented Jan 31, 2022 •
edited
Loading

This PR fixes #1461.

New feature description

This updates the design of the subdomain module on the premium onboarding flow for users who already have a pre-existing subdomain.

Screenshot (if applicable)

Mobile view:
image

Desktop view:
image

How to test

Have an with a premium account with a subdomain set, on the inspect tool turn off the display: none property on .c-multipart-premium-onboarding and .c-premium-onboarding-step.

Checklist

  • l10n dependencies have been merged, if any.
  • All acceptance criteria are met.
  • All UI revisions follow the coding standards, and use Protocol tokens where applicable (see /static/scss/libs/protocol/css/includes/tokens/dist/index.scss).
  • Commits in this PR are minimal and have descriptive commit messages.

@codemist codemist self-assigned this Jan 31, 2022
@codemist codemist requested a review from maxxcrawford January 31, 2022 19:44
Vinnl
Vinnl requested changes Feb 1, 2022
View reviewed changes
Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

This appears to introduce a bug; see inline comment.

Vinnl added a commit that referenced this pull request Feb 2, 2022
@Vinnl
Align chosen subdomain display with designs
b836860 
React re-implementation of
#1513.
@codemist codemist requested a review from Vinnl February 2, 2022 17:32
@codemist
Copy link
Collaborator Author

codemist commented Feb 2, 2022

Vincent's comment via Slack:
So it might just be that that's no longer necessary? I haven't dived into that code too closely, but I think the previous preview worked by inserting the chosen domain through JavaScript (i.e. that line, which now breaks), whereas you're inserting the chosen domain directly in the back-end. If the latter replaces the former, then it might just be a matter of removing the former.

Vinnl
Vinnl reviewed Feb 3, 2022
View reviewed changes
Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

Unfortunately I encountered another bug. I've described the cause inline, but you can reproduce it yourself if you register a subdomain during the onboarding flow, rather than beforehand. Could you give that a try?

privaterelay/templates/includes/premium-onboarding.html Outdated
Comment on lines 64 to 66
<p class="c-premium-onboarding-subdomain">@{{ user_profile.subdomain }}</p>
<span class="c-premium-onboarding-mozmail">.mozmail.com</span>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I'm running into another error, and I see why you're not running into it (and not in the other one either): they only occur when you claim a subdomain during the onboarding, not if you already have a subdomain before starting the onboarding. That's presumably also why there was JavaScript to set the subdomain in there, rather than just in the Django template.

This is what it looks like for me after registering a subdomain:

image

The reason for that is that at the time the back-end serves up this page, user_profile.subdomain is not set yet. And when the subdomain is registered, the page is not reloaded, so the contents of this element is still @None.mozmail.com when it is made visible.

Copy link
Collaborator Author

@codemist codemist Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ohh! I understand now, thank you so much. I re-added the Javascript that I deleted as well as the .js-premium-onboarding-domain-registration-preview class back into the module. I hope that works now, it looks fine on my end.

@codemist codemist requested a review from Vinnl February 3, 2022 18:59
codemist
codemist commented Feb 3, 2022
View reviewed changes
static/js/domain-registration.js
@@ -189,7 +189,7 @@
modalRegistrationSuccessState.classList.remove("is-hidden");

const domainPreview = document.querySelector(".js-premium-onboarding-domain-registration-preview");
domainPreview.textContent = domain + ".mozmail.com";
domainPreview.textContent = domain;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed ".mozmail.com" here since I'm adding that string directly into the HTML.

Vinnl
Vinnl reviewed Feb 4, 2022
View reviewed changes
Copy link
Collaborator

@Vinnl Vinnl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Works now! Only thing is that the @ is placed on a separate line, and it look a bit weird on desktop view where its width seems to be restricted for no particular reason (though maybe that looks less weird if the @ is not on a separate line):

image

maxxcrawford
maxxcrawford previously requested changes Feb 4, 2022
View reviewed changes
Copy link
Contributor

@maxxcrawford maxxcrawford left a comment

Choose a reason for hiding this comment

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

Reviewed!

privaterelay/templates/includes/premium-onboarding.html Outdated Show resolved Hide resolved
static/scss/partials/onboarding.scss Show resolved Hide resolved
static/scss/partials/onboarding.scss Outdated Show resolved Hide resolved
@codemist codemist force-pushed the subdomain-module-onboarding branch from a649980 to 1d79845 Compare February 9, 2022 18:11
@codemist codemist force-pushed the subdomain-module-onboarding branch from 1d79845 to 44469f3 Compare February 9, 2022 19:05
@codemist codemist merged commit ee0ae02 into main Feb 9, 2022
Vinnl added a commit that referenced this pull request Mar 15, 2022
@Vinnl
Align chosen subdomain display with designs
b256105 
React re-implementation of
#1513.
Vinnl added a commit that referenced this pull request Mar 16, 2022
@Vinnl
Align chosen subdomain display with designs
cb8b509 
React re-implementation of
#1513.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vinnl Vinnl Vinnl approved these changes

@maxxcrawford maxxcrawford Awaiting requested review from maxxcrawford

Assignees

@codemist codemist

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@codemist @Vinnl @maxxcrawford