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: implement profile page #28

Merged
merged 6 commits into from
Jul 5, 2021
Merged

feat: implement profile page #28

merged 6 commits into from
Jul 5, 2021

Conversation

smeijer
Copy link
Collaborator

@smeijer smeijer commented Jul 2, 2021

I've added some colors and content to the profile page, but I'm waiting for response from Gil for the QR code implementation.

@netlify
Copy link

netlify bot commented Jul 2, 2021

✔️ Deploy Preview for kcd-react ready!

🔨 Explore the source changes: 2cdbb42

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

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

app/routes/me.tsx Outdated Show resolved Hide resolved
app/routes/me.tsx Outdated Show resolved Hide resolved
app/routes/me.tsx Outdated Show resolved Hide resolved

function LogoutIcon() {
return (
<svg width="24" height="24" fill="none" viewBox="0 0 24 24">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TIL that the icons are coming from https://iconic.app. Note to self, replace the other icons as well, as those were copied from figma.

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.

Looks excellent so far 👍

app/routes/me.tsx Outdated Show resolved Hide resolved
app/routes/me.tsx Outdated Show resolved Hide resolved
<div className="col-span-full mb-12 lg:col-span-5 lg:col-start-8 lg:mb-0">
<H2 className="mb-2">Need to login somewhere else?</H2>
<H2 variant="secondary" as="p">
Scan this QR code on the other device.
Copy link
Collaborator Author

@smeijer smeijer Jul 3, 2021

Choose a reason for hiding this comment

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

Still waiting, but added something creative in the mean while.

Copy link
Owner

Choose a reason for hiding this comment

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

I like the way it looks. Technically it's challenging though because I don't want the QR code to show up without the user clicking it for two reasons:

  1. I want the QR code to expire after a short period
  2. I don't want someone else to be able to scan it over the person's shoulder

So it should be hidden until the user clicks something and when they do we'll make a request to generate it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. How about adding an overlay mentioning "click to reveal", exactly where the image is right now. And then when clicked, we fade that overlay out, and the QR in?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh yeah, good idea. And maybe we could put a little 60 second countdown timer in the middle of the QR code. Once the time is up it fades back out and you have to generate a new one. We'll also want a button to hide it earlier. This is all the "fun" and "extra" stuff so if you have other things you want to get done sooner go ahead and just keep this bit out for now.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, I'll just merge this as-is. We can come back to this later.

@smeijer smeijer marked this pull request as ready for review July 3, 2021 15:28
app/routes/me.tsx Outdated Show resolved Hide resolved
@smeijer smeijer self-assigned this Jul 4, 2021
@smeijer
Copy link
Collaborator Author

smeijer commented Jul 5, 2021

I think this one is ready to get merged. I've marked the email input as being read-only, and Gil has let me know that he's okay with the way I've implemented the QR code. He did mention that it can become a bit large. At the same time, I think that's a good thing, as not all phones have super awesome cameras, and the QR code is one of high density.

That being said, I have created a note on the project board so we can think about it for a moment, and maybe come back to it (and make it smaller). https://github.com/kentcdodds/remix-kentcdodds/projects/1#card-64414167

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.

Great progress here 👍

<div className="col-span-full mb-12 lg:col-span-5 lg:col-start-8 lg:mb-0">
<H2 className="mb-2">Need to login somewhere else?</H2>
<H2 variant="secondary" as="p">
Scan this QR code on the other device.
Copy link
Owner

Choose a reason for hiding this comment

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

I like the way it looks. Technically it's challenging though because I don't want the QR code to show up without the user clicking it for two reasons:

  1. I want the QR code to expire after a short period
  2. I don't want someone else to be able to scan it over the person's shoulder

So it should be hidden until the user clicks something and when they do we'll make a request to generate it.

@kentcdodds kentcdodds merged commit b0b07fe into main Jul 5, 2021
@kentcdodds kentcdodds deleted the feature/profile branch July 5, 2021 19:57
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