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

feat: add login page #24

Merged
merged 9 commits into from Jul 1, 2021
Merged

feat: add login page #24

merged 9 commits into from Jul 1, 2021

Conversation

smeijer
Copy link
Collaborator

@smeijer smeijer commented Jul 1, 2021

Polished the login page 😎

@netlify
Copy link

netlify bot commented Jul 1, 2021

✔️ Deploy Preview for kcd-react ready!

🔨 Explore the source changes: ae287d3

🔍 Inspect the deploy log: https://app.netlify.com/sites/kcd-react/deploys/60de1aa8d227410007e955e5

😎 Browse the preview: https://deploy-preview-24--kcd-react.netlify.app

@kentcdodds
Copy link
Owner

Looks super! I'll look at the code soon, but I had a few thoughts on the visuals.

Can you figure out why autofill makes it look so bad (both on light and dark mode):

image

I'm not 100% certain I prefer this over the gray (teamless) Alex profile. What do you think?

image

image

Rather thank making the entire form disappear, I think I'd rather render this message in the same place we put the error message:

image

I especially like including the ✨ because the magic link email includes it so it helps create cohesion. What do you think?

@kentcdodds
Copy link
Owner

One other note about the teamless Alex profile photo. When a user is logged in, if they don't have a photo then we fallback to Alex in their team color, so I think that creates cohesion.

@smeijer
Copy link
Collaborator Author

smeijer commented Jul 1, 2021

True, but it can also be confusing. It might give the user the impression they're logged in when they're not. Let's roll back Alex, and see if we get messages from confused users?

Regarding the autofill thing, I'll try fix it, but it will be hacky. Browsers don't really support customizing the autofill style. Which sucks.

@smeijer
Copy link
Collaborator Author

smeijer commented Jul 1, 2021

Regarding Alex again, desktop resolution labels that button as Sign up (I need to add that). That makes switching to Alex on mobile resolution a bit confusing as well. But the user icon doesn't really feel like a "sign up" either 🤔 .

image

@kentcdodds
Copy link
Owner

I think that design was made before we had avatars

@smeijer
Copy link
Collaborator Author

smeijer commented Jul 1, 2021

  • overruled the autofill colors
  • restored Alex on both mobile and desktop resolutions
  • rendered the success message inline

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Looking great!

app/components/icons/user-icon.tsx Outdated Show resolved Hide resolved
app/components/form-elements.tsx Outdated Show resolved Hide resolved
@@ -1,3 +1,30 @@
:focus:not(:focus-visible) {
outline: none;
}

Copy link
Owner

Choose a reason for hiding this comment

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

lol, what a mess we have to deal with here 😆

Thanks!

styles/app.css Outdated
input:-webkit-autofill:focus,
input:-webkit-autofill:active {
-webkit-text-fill-color: black !important;
-webkit-box-shadow: 0 0 0 999px #e6e9ee inset !important; /* gray-200 */
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we should create a css variable so this and gray-200 can use the same thing (also allows us to change it for light/dark mode as needed).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Somewhat done with ae287d3. I think we can build upon that to improve things futher. Think of slimming down the gray-pallete, and assign colors like. I need to research a bit, but I think we can reduce our current 8 gray colors, to 4 (per theme) by doing so.

<div className="bg-gray-dark" />

instead of:

<div className="bg-gray-200 dark:bg-gray-800" />

styles/app.css Outdated Show resolved Hide resolved
@@ -44,6 +44,10 @@ module.exports = {
500: '#4B96FF',
100: '#E8F2FF',
},
red: {
// TODO: decide if this is a good color, I just took team-red
500: '#FF4545',
Copy link
Owner

Choose a reason for hiding this comment

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

Seems legit to me. I'd like to hear what Gil has to say though 👍

@kentcdodds
Copy link
Owner

Sorry about the conflict. I resolved it for you.

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Super duper 👍

@kentcdodds kentcdodds merged commit e977628 into main Jul 1, 2021
@kentcdodds kentcdodds deleted the feature/login branch July 1, 2021 19:53
@smeijer smeijer self-assigned this Jul 4, 2021
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.

None yet

2 participants