-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add sign up dialog for beta tester list #248
Conversation
Preview build will be at |
</form> | ||
</div> | ||
<div class="flex items-center justify-end gap-x-5"> | ||
<StandardButton onClick={onClose}>{$t('sign-up.skip-action')}</StandardButton> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm whether this is a link-like button in the bottom left or a standard secondary-styled button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise LGTM
I think the content of the dialog looks somehow a bit off somehow 🤔
Maybe having the hammer on the left is better and the input field the width of the text and image? What do you think @microbit-matt-hillsdon ?
Version with the hammer on the left is definitely an improvement. |
{ | ||
type: 'local', | ||
domain, | ||
category: 'essential', | ||
name: 'hasSeenAppVersionRedirectDialog', | ||
purpose: | ||
'Used to ensure that the UK primary school teacher dialog is only shown once', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@microbit-grace your draft PR for maint-nextgen will also need this. Maybe we should agree the language with Matt first as I'm not convinced the purpose fields are correct for either.
Seems hard to make it look good with an icon like this. |
There's a timing issue where the sign-up dialog shows behind the teacher choice (briefly?) and then the local storage state is wrong so it doesn't show afterwards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.