Implement UI for certificate #232

Ririio · 2023-02-16T00:50:09Z

This pr is based on issue #213

Here's a live demo of the UI:

2023-02-15.19-37-31.mp4

I decided to use an Accordion instead of a textArea, I find it a lot more cleaner
I used useState to display different components once the user has pressed the Request a Certificate button
Instead of having a large download button for each key, I decided to switch it to a button that has hover effect, tooltip and gap, to ensure that user don't accidentally press the button
I'm unable to access the seed certificate, so I'm using a temporary object to produce the necessary output

ghost

I think the loading progress is not the correct UX here.

A certificate request will probably take at the very minimum 1-2 minutes, users will not wait that long for that animation (it looks cool for sure, but still)

Also, it feels like it tries to keep the user glued to the screen, but the process is unfortunately not that fast for this to be feasible.

This is why we talked about showing some kind of pending state in the same structure we are showing the certificate details in the second screen in your video

ghost · 2023-02-16T03:36:34Z

On the other hand, the cert details screen looks very nice :)

Ririio · 2023-02-16T03:40:32Z

I think the loading progress is not the correct UX here.

A certificate request will probably take at the very minimum 1-2 minutes, users will not wait that long for that animation (it looks cool for sure, but still)

Also, it feels like it tries to keep the user glued to the screen, but the process is unfortunately not that fast for this to be feasible.

This is why we talked about showing some kind of pending state in the same structure we are showing the certificate details in the second screen in your video

Yeah... I'm struggling a bit with understanding what a good UX is for the loading page. From what I can understand, you want to head directly to the 2nd page, but provide some sort of a "loading" buffer before displaying the details? Like a spinning wheel of some kind?

ghost · 2023-02-16T14:20:25Z

Yes, with some kind of description, that we are creating the certificate, but this is an asyncrhronous process, requiring multiple 3rd party providers (route53 and let's encrypt), so for this reason it will take a few minutes

ghost · 2023-02-16T14:20:57Z

also a note that the user can leave the page and come back later, the process will continue in the background

Ririio · 2023-02-16T14:22:27Z

The loading page is meant to stay regardless on whether the user refresh the page or not. The state will only change after the certificate becomes valid (at least that's how I thought it would work...). Right now it's handled by a timeout

humphd · 2023-02-16T18:21:06Z

I really love this @Ririio, great job. A few comments:

For the "pending" screen, I think it should be static (i.e., no animation), which will help with it taking a while and not seeming broken. We could include an image from https://undraw.co/illustrations coloured red for example. Not this one, but something like this with text saying, "We've ordered your certificate and will notify you when it's ready":

We said that the main part of the UI should be centred vs. left-aligned.

Ririio · 2023-02-16T21:21:05Z

For the "pending" screen, I think it should be static (i.e., no animation), which will help with it taking a while and not seeming broken. We could include an image from https://undraw.co/illustrations coloured red for example. Not this one, but something like this with text saying, "We've ordered your certificate and will notify you when it's ready":

We're allowed to use this website for illustration? I

We said that the main part of the UI should be centred vs. left-aligned

I recall there was a discussion for the ui to be left-aligned-- I had to change the layout on the figma template

humphd · 2023-02-16T21:39:50Z

We're allowed to use this website for illustration?

Yes, it is open licensed

I recall there was a discussion for the ui to be left-aligned-- I had to change the layout on the figma template

What I heard in that meeting was "left-aligned in a container that is centred," but if I'm wrong, do what you have.

cc @Myrfion, @Eakam1007

Eakam1007 · 2023-02-16T21:55:43Z

What I heard in that meeting was "left-aligned in a container that is centered," but if I'm wrong, do what you have.

Yeah, I believe it was discussed that the form should not touch the left edge but should be a bit more towards the center (so basically a margin on the left)

I think this suggestion should make it work

Ririio · 2023-02-16T22:08:25Z

Yeah, I believe it was discussed that the form should not touch the left edge but should be a bit more towards the center (so basically a margin on the left)

Just to be clear, I just have to move the container more to the center, and have its content left align?

Eakam1007 · 2023-02-16T22:17:26Z

Just to be clear, I just have to move the container more to the center, and have its content left align?

I would say it is up to you how you decide to do it. But yeah, I think that should work

humphd · 2023-02-16T22:53:59Z

What I'm more looking for is that this be responsive. When we're on a really wide screen, it's nicer to put it in the middle. On a narrower screen, it's going to be more on the left, etc.

We can also do that stuff in a follow up. But putting the "main" part of this in a container that is in the center, and the contents are left-aligned within it, seems good to me.

If it doesn't work, don't do it. Just a thought.

Ririio · 2023-02-17T00:04:24Z

@humphd @dadolhay I changed the loading screen, and aligned the containers in the middle as per your suggestion. If you don't mind taking a look here's a demo...

2023-02-16.19-00-15.mp4

humphd

I really like where this is going.

Reading the code, I wonder if we should be splitting this up into a few larger components vs. having such deep nesting on state. That could happen in a follow-up, so we don't block this for 0.3, but I think this file is getting pretty big, and we could separate the three states into smaller pieces of code that would be easier to maintain.

I'm tagging some other reviewers who can help with thinking about the front-end stuff.

Ririio · 2023-02-17T03:12:11Z

Reading the code, I wonder if we should be splitting this up into a few larger components vs. having such deep nesting on state. That could happen in a follow-up, so we don't block this for 0.3, but I think this file is getting pretty big, and we could separate the three states into smaller pieces of code that would be easier to maintain.

If we're splitting them up, can I create a directory called components underneath the certificate to split the code? I'm not familiar with the standard practice of remix in terms of creating components, so I refrained from doing it. Am I allowed to do something like this?

Eakam1007 · 2023-02-17T03:34:21Z

I believe there already is a components directory under app. I think you could use that?

Ririio · 2023-02-17T03:45:05Z

I believe there already is a components directory under app. I think you could use that?

sure

Ririio · 2023-02-17T04:00:35Z

@Eakam1007 is this what you meant?

humphd

certificate-display.tsx does the exact same UI twice. You could break that up into re-usable pieces too, which will halve this code, and pass the things that are different (e.g., the title, the value in the accordion view) as props.

Because you have a bunch of "certificate" components (e.g., loading is pretty vague, I'd put them all under app/components/certificate/*` so they are logically grouped together. Then you can break out smaller bits without it getting overwhelming.

In your certificate/index.tsx, don't be afraid to use an if:

if(loading) {
  return <Loading />
}

if(certificate) {
  return <CertificateDetails certificate={certificate} />
}

return <RequestCertificate />

humphd · 2023-02-17T15:04:21Z

app/components/certificate/certificate-display.tsx

+    let key = props.cert.certificate;
+    let val = 'Public Key';
+
+    if (props.priv) {


Instead of doing this, pass down the title to show (e.g., 'Private Key') and also the value to use (i.e., the specific PEM string to show). This component shouldn't have to know anything about what it's showing, do that above it.

I'm a bit confused as to what you mean, do you mind explaining it one more time?

humphd · 2023-02-17T15:05:19Z

app/components/certificate/certificate-display.tsx

+          <Heading as="h4" size="sm">
+            {props.priv ? 'Private Key' : 'Public Key'}
+          </Heading>
+          <Tooltip label="Copy Public Key">


This public vs. private logic should get figured out via props vs hard-coding like this and below.

I decided to create a switch and 3 new variables that will be used for the key, currentState and Description. This should remove any need for hard coding, because the output is dependent on the current state of the isPrivate props

humphd · 2023-02-17T15:05:25Z

app/components/certificate/certificate-display.tsx

+              onClick={() => onCopy()}
+            />
+          </Tooltip>
+          <Tooltip label="Download Public Key">


Should I create a variable that changes depending on the state of isPrivate?

app/components/certificate/certificate-display.tsx

app/components/certificate/description.tsx

humphd · 2023-02-17T15:07:10Z

app/components/certificate/description.tsx

+        <HStack gap="6" marginTop="2">
+          <VStack>
+            <Text fontWeight="bold">Created On</Text>
+            <Text>{props.cert.validFrom.toDateString()}</Text>


Use chakra.time for a <time> element, and use the datatime attribute to set a machine readable value.

I'm sorry I can't seem to find the document relating to chakra.time do you happen to have the link for it?

humphd · 2023-02-17T15:07:54Z

app/components/certificate/loading.tsx

@@ -0,0 +1,18 @@
+import { Center, Flex, Box, Image, Heading } from '@chakra-ui/react';
+
+import image from 'img/undraw_Processing_re_tbdu.png';


I'm unclear how Remix handles assets. Are these supposed to go in public/ or in the tree like this?

We'll also need to include the license for this image, or we are breaking the license.

I'm also not sure... Alex might know a bit more about it, because she has also have files on the img directory

I was looking for someone to go read the docs, which are here https://remix.run/docs/en/v1/guides/migrating-react-router-app#asset-imports.

Another question: does undraw not supply these as SVGs? They will scale much better if so.

humphd · 2023-02-17T15:08:18Z

app/components/certificate/request.tsx

+import type { Certificate } from '@prisma/client';
+
+interface Props {
+  cert: Certificate;


Pass the specific thing down you want to show: domainName

I am not quite sure what you mean by this?

app/routes/__index/certificate/index.tsx

humphd · 2023-02-17T15:10:15Z

app/routes/__index/certificate/index.tsx

-      </Heading>
-    </div>
-  );
+  const [request, setRequest] = useState(false);


What does request represent? Would this not be certificate (i.e., you'd show the certificate display only if you have a certificate)?

the 'request' is a state when the certificate has already been requested (true) or not (false). This helps when rendering pages depending on whether we have a new certificate. If you have a different idea I can change the current iteration

If this is whether or not the certificate has been requested, use that: certificateRequested

Myrfion

Hi, PR looks good! I added some minor change requests that would improve code quality in my opinion

app/components/certificate/certificate-display.tsx

app/routes/__index/certificate/index.tsx

app/components/certificate/request.tsx

app/components/certificate/description.tsx

Eakam1007

Could you please add e2e tests for this or create a issue for them?

app/components/certificate/certificate-display.tsx

app/components/certificate/description.tsx

humphd · 2023-02-19T13:39:22Z

app/components/certificate/loading.tsx

@@ -0,0 +1,18 @@
+import { Center, Flex, Box, Image, Heading } from '@chakra-ui/react';
+
+import image from 'img/undraw_Processing_re_tbdu.png';


I was looking for someone to go read the docs, which are here https://remix.run/docs/en/v1/guides/migrating-react-router-app#asset-imports.

app/components/certificate/request.tsx

humphd · 2023-02-19T13:41:38Z

app/routes/__index/certificate/index.tsx

-      </Heading>
-    </div>
-  );
+  const [request, setRequest] = useState(false);


If this is whether or not the certificate has been requested, use that: certificateRequested

humphd · 2023-02-19T13:42:11Z

app/components/certificate/loading.tsx

@@ -0,0 +1,18 @@
+import { Center, Flex, Box, Image, Heading } from '@chakra-ui/react';
+
+import image from 'img/undraw_Processing_re_tbdu.png';


Another question: does undraw not supply these as SVGs? They will scale much better if so.

app/components/certificate/certificate-display.tsx

humphd · 2023-02-19T19:50:24Z

app/components/certificate/certificate-display.tsx

+}
+
+export default function CertificateDisplay(props: CertificateDisplayProps) {
+  const { title, description, value } = props;


nit:

export default function CertificateDisplay({ title, description, value }: CertificateDisplayProps) {

oh that makes it much better than calling it again inside

humphd · 2023-02-19T19:50:44Z

app/components/certificate/certificate-display.tsx

+  function onCopy() {
+    navigator.clipboard.writeText(value);
+    toast({
+      title: `${title} was copied to clipboard`,


"the clipboard"

app/components/certificate/loading.tsx

humphd · 2023-02-19T19:56:20Z

app/routes/__index/certificate/index.tsx

+            />
+          </>
+        )}
+        {!certificateRequested && (


This pattern feels off to me. We have a state that will result in one of two component sub-trees being rendered in this view. I think you should make two components and do:

{ certificateRequested ? <View1 /> : <View2 /> }

Maybe name them:

CertificateRequested

CertificateAvailable

There might be something better, not sure.

I actually thought about doing this on my previous commits, but I can't seem to figure out how I'll handle the button for request certificate

oh it's actually fairly simple to do...

humphd

This is looking better and better. A few more things

app/components/certificate/certificate-available.tsx

app/components/certificate/certificate-request.tsx

app/components/certificate/description.tsx

humphd

One small change, and this is r+ from me

app/components/certificate/certificate-request.tsx

replacing privkey with privateKey center components | adding unDraw illustration creating 'loading' and 'certificate-display' components creating new components for certificate Implementing advice from reviews Changing function name to be more descriptive passing props instead of using logic in child component Trimming the index to only contain two components as to a tree all logic should occur in the parent component following react event handler prefix

Outdated

Eakam1007

The page is not really responsive:

Though this can be addressed in another issue, if you would like to create one. And same goes for e2e tests for this

Otherwise, looks good

Ririio added category: front end Front end part of our web service area: web Web development related things [front end/back end] labels Feb 16, 2023

Ririio added this to the Milestone 0.3 milestone Feb 16, 2023

Ririio self-assigned this Feb 16, 2023

Ririio force-pushed the issue-213 branch from 6b951fb to c60f152 Compare February 16, 2023 00:56

ghost previously requested changes Feb 16, 2023

View reviewed changes

Ririio force-pushed the issue-213 branch from c60f152 to d362887 Compare February 17, 2023 00:02

humphd requested a review from Myrfion February 17, 2023 00:18

humphd reviewed Feb 17, 2023

View reviewed changes

humphd requested a review from Eakam1007 February 17, 2023 00:22

Ririio force-pushed the issue-213 branch from d362887 to 135cbbc Compare February 17, 2023 03:58

Ririio requested a review from a user February 17, 2023 04:02

humphd requested changes Feb 17, 2023

View reviewed changes

Ririio force-pushed the issue-213 branch from 135cbbc to e6422fe Compare February 17, 2023 14:25

Ririio requested a review from humphd February 17, 2023 14:37

humphd requested changes Feb 17, 2023

View reviewed changes

Myrfion previously requested changes Feb 17, 2023

View reviewed changes

SerpentBytes modified the milestones: Milestone 0.3, Milestone 0.4 Feb 19, 2023

Ririio requested review from Myrfion and humphd February 19, 2023 03:49

Eakam1007 reviewed Feb 19, 2023

View reviewed changes

humphd requested changes Feb 19, 2023

View reviewed changes

Ririio requested a review from humphd February 19, 2023 14:48

humphd requested changes Feb 19, 2023

View reviewed changes

Ririio requested a review from humphd February 19, 2023 21:10

humphd requested changes Feb 19, 2023

View reviewed changes

Ririio requested a review from humphd February 19, 2023 21:42

humphd requested changes Feb 19, 2023

View reviewed changes

app/components/certificate/certificate-request.tsx Outdated Show resolved Hide resolved

Ririio force-pushed the issue-213 branch from 5055c8f to fcbadf5 Compare February 19, 2023 21:58

Ririio requested a review from humphd February 19, 2023 21:59

humphd approved these changes Feb 19, 2023

View reviewed changes

Ririio requested a review from Eakam1007 February 19, 2023 22:01

Eakam1007 approved these changes Feb 20, 2023

View reviewed changes

Ririio merged commit ab1dad8 into DevelopingSpace:main Feb 20, 2023

Ririio deleted the issue-213 branch February 25, 2023 13:20

		@@ -0,0 +1,18 @@
		import { Center, Flex, Box, Image, Heading } from '@chakra-ui/react';

		import image from 'img/undraw_Processing_re_tbdu.png';

Implement UI for certificate #232

Implement UI for certificate #232

Conversation

Ririio commented Feb 16, 2023 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

ghost commented Feb 16, 2023

Ririio commented Feb 16, 2023

ghost commented Feb 16, 2023

ghost commented Feb 16, 2023

Ririio commented Feb 16, 2023 • edited Loading

humphd commented Feb 16, 2023 • edited Loading

Ririio commented Feb 16, 2023

humphd commented Feb 16, 2023

Eakam1007 commented Feb 16, 2023 • edited Loading

Ririio commented Feb 16, 2023

Eakam1007 commented Feb 16, 2023

humphd commented Feb 16, 2023

Ririio commented Feb 17, 2023 • edited Loading

humphd left a comment

Choose a reason for hiding this comment

Ririio commented Feb 17, 2023

Eakam1007 commented Feb 17, 2023

Ririio commented Feb 17, 2023

Ririio commented Feb 17, 2023

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ririio Feb 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ririio Feb 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ririio Feb 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myrfion left a comment

Choose a reason for hiding this comment

Eakam1007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Eakam1007 left a comment

Choose a reason for hiding this comment

Ririio commented Feb 16, 2023 •

edited

Loading

Ririio commented Feb 16, 2023 •

edited

Loading

humphd commented Feb 16, 2023 •

edited

Loading

Eakam1007 commented Feb 16, 2023 •

edited

Loading

Ririio commented Feb 17, 2023 •

edited

Loading

Ririio Feb 19, 2023 •

edited

Loading

Ririio Feb 19, 2023 •

edited

Loading

Ririio Feb 19, 2023 •

edited

Loading