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

[OMN-189] : Settings View - Emails #286

Merged
merged 9 commits into from Mar 23, 2022
Merged

[OMN-189] : Settings View - Emails #286

merged 9 commits into from Mar 23, 2022

Conversation

gitstart-omnivore
Copy link
Contributor

@gitstart-omnivore gitstart-omnivore commented Mar 22, 2022

Spec Checklist

  • Are Api endpoints ready and properly documented ?
  • Does the figma design fit the ticket description ?
  • Added video describing your plan of action ?

Description

This is part of the base issue Settings View

Omnivore allows users to create addresses that receive email and add it to a user's inbox. A user can create multiple email addresses. There is a temporary version of this page available at settings/emails.

image

A new design for this page has been created here:

https://www.figma.com/file/ILgs6aXrPfukXOMf5Yibra/Omnivore---Deliverables?node-id=444%3A33319

Notes
The design for this view is slightly off. Visually it is correct, but the user experience is inaccurate. Email addresses are immutable. They can be created, and they can be deleted, but they can not be modified. Also, they are created by Omnivore's backend, so there should be no user input on this page.

The confirmation codes are created when a GMail address is linked with an Omnivore Inbox address. So in testing they will normally not be set.

[ ] List emails

[ ] Display confirmation code if set on an email

[ ] Create an email

[ ] Delete an email

[ ] Copy an email address

[ ] Copy a confirmation code

[ ] Light and dark mode support

[ ] Desktop and mobile support

Loom

loom video

House Keeping

  • Added video describing your plan of action ?
  • Added Loom video ?
  • Is Deployment Link passing ?
  • Have you assinged PR to yourself ?
  • Is Github action passing ?
  • Updated the appropriate tag ?

Ticket link (if applicable)

How has this been tested?

Types of changes

  • Bug fix
  • New feature
  • Refactor
  • Others

Checklist:

  • I have test my code on different browsers and devices to make sure it is cross-browser compatible (both desktop and mobile).
  • I have confirmed the payload for integrated API matches params on API docs
  • I have confirmed design matches Spec specified on Figma.

Remarks

  • I added a new package to achieve this task.
  • I have updated package.json

@vercel
Copy link

vercel bot commented Mar 22, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

omnivore-demo – ./

🔍 Inspect: https://vercel.com/omnivore/omnivore-demo/2ELiLTyxCMGXdy83rGTg5sKW93rr
✅ Preview: https://omnivore-demo-git-omn-189-omnivore.vercel.app

omnivore-prod – ./

🔍 Inspect: https://vercel.com/omnivore/omnivore-prod/HShE2WHLDEQss2CMgdHbPTjStYaU
✅ Preview: https://omnivore-prod-git-omn-189-omnivore.vercel.app

@jacksonh
Copy link
Contributor

Deploying this one to the demo environment.

@jacksonh
Copy link
Contributor

On mobile can we add padding above the table. Its a little too tight to the top of the page

Here's the designed version:

Screen Shot 2022-03-21 at 21 32 59

and here is the actual:

Screen Shot 2022-03-21 at 21 31 41

@jacksonh
Copy link
Contributor

On mobile because there is no header, we need to round the top corners of the first item. You can see in our version we have straight lines on the top row on mobile.

@jacksonh
Copy link
Contributor

On mobile can we add a little more padding to the two right side buttons. I know that will truncate the address but I think its a little too tight without.

Screen Shot 2022-03-21 at 21 39 32

@jacksonh
Copy link
Contributor

On mobile the bottom button is too tight on the bottom of the page.

Screen Shot 2022-03-21 at 21 42 17

@@ -7,48 +9,9 @@ type MoreOptionsIconProps = {
}

export function MoreOptionsIcon(props: MoreOptionsIconProps): JSX.Element {
const dots =
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

@@ -23,6 +23,7 @@
"@radix-ui/react-id": "^0.1.1",
"@radix-ui/react-popover": "^0.1.1",
"@radix-ui/react-separator": "^0.1.0",
"@radix-ui/react-tooltip": "^0.1.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -48,7 +49,9 @@ function OmnivoreApp({ Component, pageProps }: AppProps): JSX.Element {
defaultLocale="en"
messages={englishTranslations}
>
<Component {...pageProps} />
<TooltipProvider delayDuration={200}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah this is handy. Should be able to use these a lot more now.

@jacksonh
Copy link
Contributor

On desktop the InfoIcon should link to this page when tapped: https://omnivore.app/help/newsletters

Copy link
Contributor

@jacksonh jacksonh left a comment

Choose a reason for hiding this comment

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

added some comments regarding visual issues, mostly on mobile

@gitstart-omnivore
Copy link
Contributor Author

@jacksonh we've effected the requested review changes, please take a look

@jacksonh
Copy link
Contributor

@jacksonh we've effected the requested review changes, please take a look

Thanks, taking a look.

@jacksonh
Copy link
Contributor

Deploying this to the demo environment

@gitstart-omnivore
Copy link
Contributor Author

This should just be the relative URL /help/newsletters

Fixed

@jacksonh
Copy link
Contributor

This looks good. I'll merge/deploy it in the morning.

@jacksonh
Copy link
Contributor

There's one last small issue here. On mobile, if you have a single email, you don't get rounded corners, because this loop here doesn't account for the case where i ===0 and isLastChild is true:

                  '@mdDown': {
                    borderRadius: i === 0 ? '5px 5px 0 0 ' : '',
                  },
                  borderRadius: isLastChild ? '0 0 5px 5px' : '',

@gitstart-omnivore
Copy link
Contributor Author

There's one last small issue here. On mobile, if you have a single email, you don't get rounded corners, because this loop here doesn't account for the case where i ===0 and isLastChild is true:

                  '@mdDown': {
                    borderRadius: i === 0 ? '5px 5px 0 0 ' : '',
                  },
                  borderRadius: isLastChild ? '0 0 5px 5px' : '',

Fixed

@jacksonh
Copy link
Contributor

nice work.

@jacksonh jacksonh merged commit 13d8039 into main Mar 23, 2022
@jacksonh jacksonh deleted the OMN-189 branch March 23, 2022 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants