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

style: make transfer ownership email more prominent #630

Merged
merged 2 commits into from
Nov 13, 2020

Conversation

tshuli
Copy link
Contributor

@tshuli tshuli commented Nov 13, 2020

Problem

Closes #499

Before & After Screenshots

BEFORE:

Screenshot 2020-11-13 at 11 07 02 AM

AFTER:

Screenshot 2020-11-13 at 11 06 14 AM

Tests

  • Open form transfer ownership. Check that the new email address is in red.

@tshuli tshuli changed the title style: warning color for email style: make transfer ownership email more prominent Nov 13, 2020
@tshuli tshuli requested a review from r00dgirl November 13, 2020 03:11
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm, but consider changing the id to a class instead

@@ -10,7 +10,8 @@
>
<div ng-if="isDisplayTransferOwnerModal">
<div class="label-custom" id="invite-title">
Transfer ownership to {{transferOwnerEmail}}
Transfer ownership to
<span id="transfer-owner-email">{{transferOwnerEmail}}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer not using ids and stick to classes. ids can only be used once on the entire layout (and whilst the odds of transfer-owner-email being used somewhere else it's still plausible haha

ids should be kept to maybe a container div/etc, not for a span.

@tshuli tshuli merged commit cb1a07e into develop Nov 13, 2020
@tshuli tshuli deleted the transfer-ownership branch November 13, 2020 12:34
@karrui karrui mentioned this pull request Nov 17, 2020
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.

Make new form owner email more prominent in the transfer ownership confirmation modal
2 participants