Per 9491 new sign in #366

crisnicandrei · 2024-03-05T14:05:34Z

This pull request serves as the new log-in/signup/forgotten password pages. This has been checkout out of the new checkbox component because we did not have it, but the design was making use of it.

Some components have slight design changes because when I integrated them into the new design, I realised they were a bit off.

codecov-commenter · 2024-03-05T14:08:54Z

Codecov Report

Attention: Patch coverage is 53.84615% with 6 lines in your changes missing coverage. Please review.

Project coverage is 42.03%. Comparing base (b65b551) to head (c81026f).
Report is 65 commits behind head on main.

Files with missing lines	Patch %	Lines
...nents/forgot-password/forgot-password.component.ts	0.00%	3 Missing ⚠️
src/app/auth/components/signup/signup.component.ts	33.33%	2 Missing ⚠️
src/app/auth/components/login/login.component.ts	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #366      +/-   ##
==========================================
- Coverage   42.04%   42.03%   -0.01%     
==========================================
  Files         346      353       +7     
  Lines       10880    10929      +49     
  Branches     1777     1783       +6     
==========================================
+ Hits         4574     4594      +20     
- Misses       6157     6184      +27     
- Partials      149      151       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

meisekimiu

This looks really good! Most of my feedback comes in the form of some functionality-level accessibility issues. Technically I should have caught some of these a bit sooner in the individual component PRs, but I was a bit more tolerant about some implementation details of these components since they weren't being used in the UI yet. Now that this is their first official usage I have some more notes. Sorry for not testing or noticing all of this earlier.

The main accessibility issue is just that these components are not very keyboard accessible. Here are some of my observations:

The checkbox component is not accessible by keyboard at all. Ideally it should be just as functional as a native checkbox. (If I were implementing the component, it would wrap around an actual textbox element so that all native functionality is still intact, even if the Angular component has additional fancy effects. There are other ways of doing this as well, though.)
The secondary type button component does not have a visible selection outline when using a keyboard, making it hard to tell if it's even focused with the keyboard.
The button components cannot be clicked with just the keyboard. This seems to be a conflict with having both a (click) event on the button as well as a [routerLink]? Maybe?

There are also some more UI-level accessibility issues that the experience team has spotted (things like color contrast, etc), although I think we can ignore those until the designs themselves are updated. (...Maybe we could underline text links though, that's not going to need real design work. Up to you if you want to implement it or wait for specific changes to be made in the design.)

One final question I have: Does this ticket include mobile designs at all or just the main tablet design? This implementation seems to be at least somewhat responsive, but there are a few weird spacing issues in the mobile view. Ideally I'd like to fix those even if mobile isn't a priority.

meisekimiu · 2024-03-13T21:22:36Z

src/app/shared/components/or-divider/or-divider.component.ts

@@ -0,0 +1,11 @@
+import { Component } from '@angular/core';


Let's put this component in the AuthModule since it's really only going to be used there. The component can be declared, but it doesn't need to be exported unless other modules need to use it in the future. In general, we want to break up the SharedModule into more specific modules (#20) so we shouldn't be adding new components to this module anymore.

meisekimiu · 2024-03-13T21:24:54Z

src/app/auth/components/forgot-password/forgot-password.spec.ts

+
+import { LogoComponent } from '@auth/components/logo/logo.component';
+
+xdescribe('MfaComponent', () => {


Suggested change

xdescribe('MfaComponent', () => {

describe('MfaComponent', () => {

This should be caught by the linting rule enabled in #369 after a rebase, but still we should not be disabling tests.

meisekimiu · 2024-03-13T21:27:46Z

src/app/component-library/components/button/button.component.scss

@@ -88,8 +93,12 @@

  &-fill {
    width: 100%;
-    padding: 0px 24px;
+    padding: 12 24px;


Should this 12 have units on it?

meisekimiu · 2024-03-13T21:54:42Z

src/app/auth/components/forgot-password/forgot-password.component.html

+  <div class="buttons">
+    <pr-button
+      [attr]="'submit'"
+      [text]="'Recover my password'"


Some additional comments on the pr-button component after seeing it used in "real" code. I think ideally it makes more sense for the pr-button element to kind of act like a normal button element and accept arbitrary child elements. So instead of passing in a text prop, we could just write <pr-button>Recover my password</pr-button>. Do you think this is overkill though? Maybe limiting the contents to just basic text is fine and even the kind of behavior we want to encourage?

meisekimiu · 2024-03-13T21:55:20Z

src/app/auth/components/forgot-password/forgot-password.component.html

+  </div>
+  <div class="buttons">
+    <pr-button
+      [attr]="'submit'"


In hindsight I think this specific input should be called type (maybe buttonType or something instead?) instead of attr. Didn't catch this earlier.

meisekimiu · 2024-03-13T22:10:48Z

Bonus tip I'm leaving in an extra comment! This is a big pull request that's probably going to have some back and forth since it's a big new UI change and I'm sure even after code review passes there will be additional QA comments. Unfortunately, that means this code is not going to be integrated into main until many people at Permanent have signed off on the releasability of this new screen.

In the future for big redesigns like this, we should try experimenting with a strategy that allows us to merge code without releasing that new screen to users yet. What I specifically mean by this is that instead of replacing the old components, we can create new components that make up this new UI and for a time both the old UI and new UI coexist in the codebase. In production the new UI is hidden away from the user, but the code is merged. Once we decide the new screen is actually releasable, we fully replace the old screen with the new one.

This isn't advisable in all situations, but I think it could speed up merging PRs on some future big tickets, which through this technique can hopefully be broken into smaller more iterative tickets that can be reviewed faster.

crisnicandrei · 2024-03-14T10:50:27Z

@meisekimiu could you please comment here the spacing issues that you. have encountered? I might overlook something, so having someone else look could help me. Otherwise all your comments are valid and I will take care of them. Also thank you for the tip! Will keep in mind when working on bigger tasks!

meisekimiu · 2024-03-15T14:36:47Z

On mobile view, the margin between the main heading and the actual text inputs is a bit too big. It requires me to actually scroll to access the inputs to log in:

Also on this screen there seems to be no padding on the body text which is inconsistent with other screens (like the terms) and makes everything a bit harder to read:

meisekimiu

I think we can improve some of the responsiveness of this layout slightly. Because of the negative space in the design right now, should we change the mobile breakpoint to be a bit bigger? The actual sign in interface gets a bit too small right before that breakpoint.

meisekimiu · 2024-03-15T14:42:09Z

src/app/auth/components/login/login.component.scss

+
+.page-title {
+  @include pageTitle;
+  margin-bottom: 175px;


This margin in particular seems to be the cause for problematic spacing in mobile. For layout purposes I think we should stray away from such fixed pixel measurements. I know using something like % units can be a bit wonky when working vertically, but might it be better to use % (or vh) to adapt to the user's vertical resolution instead? Alternatively maybe some kind of space-between-justified flexbox?

I mainly think measurements like this should be more responsive in general.

crisnicandrei · 2024-03-15T15:14:10Z

@meisekimiu Thanks! Great inputs! I left the margin that big to be compliant with the desktop view, but I think you are right. I have pushed the changes. I have also added some padding to the form on the forgot password page and made the form bigger before it hits the tablet breakpoint

meisekimiu

Some additional comments from a second round a review.

The checkboxes appear to be a bit off center in my default zoom level in Chrome; this seems to happen around the 1340px screen width:

This seems to be a small bug that happens only at specific screen widths.

meisekimiu · 2024-03-27T19:50:10Z

src/app/auth/components/signup/signup.component.html

+<div class="terms-container" [hidden]="!showTerms">
+  <p class="terms-title">Terms & Conditions</p>
+  <p class="terms-content">
+    Welcome to The Permanent Legacy Foundation. These terms and conditions
+    outline the rules and regulations for the use of The Permanent Legacy
+    Foundation's App. The Permanent Legacy Foundation is located at: 4611 Bee
+    Caves Road, Suite 109, Austin, TX 78746 United States. Please read these
+    Terms of Service ("Terms", "Terms of Service") carefully before using the
+    app (the "Service") operated by The Permanent Legacy Foundation. ("us",
+    "we", or "our"). Your access to and use of the Service is conditioned on
+    your acceptance of and compliance with these Terms. These Terms apply to all
+    visitors, users and others who access or use the Permanent App...
+  </p>
+  <div class="terms-buttons">
+    <pr-button
+      [icon]="'decline'"
+      [orientation]="'right'"
+      (buttonClick)="acceptTerms(false)"
+      >Decline</pr-button
+    >
+    <pr-button
+      (buttonClick)="acceptTerms(true)"
+      [icon]="'accept'"
+      [orientation]="'right'"
+      [variant]="'secondary'"
+      >Accept</pr-button
+    >
+  </div>
+</div>


I think we might want to split this into another component, but before actually doing that there's a design question here for QA as well. Do we want to include the terms of service in the application itself? We did this previously and removed it so that we could link to the terms on the marketing site specifically. I think we might want to continue doing that so that we only have one copy of the terms of service, and we don't have to do a code deploy if we need to edit them.

meisekimiu · 2024-03-27T19:58:17Z

src/app/auth/components/signup/signup.component.html

-        >terms</a
-      >
-      <strong> (required)</strong>
+      <span class="terms" (click)="displayTerms()">terms</span>


This link is not keyboard accessible. I recommend making this a standard link or button that users can click using a keyboard. There's also a QA question here about whether or not we want to change this link text to be accessible as well (should we add an aria-label for this text or should we just reword the text in general.

meisekimiu · 2024-03-27T20:07:13Z

src/app/component-library/components/checkbox/checkbox.component.html

+  (keydown)="handleKeyDown($event)"
+  role="checkbox"
+  [attr.tabindex]="disabled ? -1 : 0"
+  [attr.aria-checked]="isChecked"
+  [attr.aria-disabled]="disabled"


If we use the checkbox role attribute here, browsers will mark the child elements as presentational (see: ARIA checkbox role descendants). Since the label is a child element, we should really only apply this role on the actual specific element that's representing the checkbox.

meisekimiu · 2024-03-27T20:10:46Z

src/app/component-library/components/checkbox/checkbox.component.ts

+    }
+  }
+
+  handleKeyDown(event: KeyboardEvent) {


Suggestion: While this does make keyboard functionality testable in automated tests, I do want to mention that if we use a native checkbox element that is simply restyled (or hidden with CSS), we do not have to write the checkbox logic ourselves and we don't have to write tests.

crisnicandrei · 2024-03-28T09:24:08Z

@meisekimiu this is how it looks for me at 1340

cecilia-donnelly · 2024-05-02T14:21:22Z

@crisnicandrei when you get back can you respond to @meisekimiu 's comments so I know which of them have been addressed? Thank you!

crisnicandrei · 2024-05-08T14:07:23Z

@meisekimiu @cecilia-donnelly I have addressed the comments, sorry forgot to respond here

cecilia-donnelly · 2024-07-16T14:12:01Z

Steps for this one:

@meisekimiu review
QA
Deploy (which will change the look of the signin page only)

meisekimiu

I left some comments on here. I think this will probably require a round of (blocking) QA, and for a PR this big I think I would like to re-review code after QA comments so I'm not going to explicitly approve it until after that, if that's okay.

meisekimiu · 2024-07-22T20:53:05Z

src/app/auth/components/auth/auth.component.html

+        >Start Exploring Now.</a
+      >
+    </div>
+    <img src="assets/img/sign_in_image.png" />


This should probably have an alt attribute. Either with a blank alt if we decide this is a purely decorative image, or proper alt text if we want to preserve that.

@k8lyn6 is there a specific alt text you might consider using?

meisekimiu · 2024-07-22T20:55:49Z

src/app/auth/components/auth/auth.component.scss

+  width: 80%;
+  height: 100%;
+
+  & > img {
+    width: 100%;
+    height: 100%;
+    object-fit: contain;
+  }


Should we have a more specific maximum/minimum height for this image? I feel like it has potential to look weird at different zoom levels, on big monitors, or close to the mobile breakpoint.

This is more a question for the QA process though.

emlesienk

Design: I think the "At Permanent" block needs some shadow. It looks too flat right now. If you need me to make any changes to the background image, let me know on slack because I have the file in canva. Do we want to round out the corners on the image so it flows better with the rest of the page? It looks too boxy with the squared-off corners. It might be worth me making a new graphic altogether - maybe a grid of the different archive profile pictures so we can remove the purple text boxes on the image. Can someone let me know the ideal size for this image?

Once we figure that out, I can get the alt-text to you. For the current image, the alt-text could be "Photo with a bust of a woman in striking hat and glasses. Public Archive Spotlight: The Unknown Faces Archive."

Also, we'll be making category / tag improvements to the blog soon so that link to the blog is likely to be invalid pretty soon. I'll get that new link once we have it.

Functionality: It lets me log in as expected, 2FA is working.

k8lyn6 · 2024-07-23T18:42:09Z

@crisnicandrei Can we try out this image instead?

crisnicandrei · 2024-07-23T20:05:12Z

@emlesienk that would be great if you could update the design. Thanks!

crisnicandrei · 2024-07-23T20:09:08Z

@k8lyn6 i have changed the picture. Let me know what you think!

crisnicandrei requested a review from meisekimiu March 5, 2024 14:05

crisnicandrei changed the base branch from main to PER-9488-glam-checkbox March 5, 2024 14:05

meisekimiu requested changes Mar 13, 2024

View reviewed changes

crisnicandrei force-pushed the PER-9488-glam-checkbox branch from 138f91a to 102e0df Compare March 14, 2024 09:42

Base automatically changed from PER-9488-glam-checkbox to main March 14, 2024 10:20

crisnicandrei force-pushed the PER-9491-new-sign-in branch from b563009 to a0aadd7 Compare March 14, 2024 11:40

crisnicandrei requested a review from meisekimiu March 14, 2024 11:49

meisekimiu reviewed Mar 15, 2024

View reviewed changes

crisnicandrei requested a review from meisekimiu March 15, 2024 15:13

meisekimiu reviewed Mar 27, 2024

View reviewed changes

crisnicandrei force-pushed the PER-9491-new-sign-in branch from 62e8cde to 17af411 Compare April 16, 2024 17:36

crisnicandrei force-pushed the PER-9491-new-sign-in branch from c7cb23f to 770ae03 Compare May 27, 2024 10:23

crisnicandrei force-pushed the PER-9491-new-sign-in branch from af25eb6 to 4d28c6f Compare June 19, 2024 10:25

cecilia-donnelly requested a review from meisekimiu July 1, 2024 16:19

meisekimiu reviewed Jul 22, 2024

View reviewed changes

meisekimiu requested a review from k8lyn6 July 22, 2024 21:00

k8lyn6 requested a review from emlesienk July 23, 2024 16:17

emlesienk suggested changes Jul 23, 2024

View reviewed changes

crisnicandrei added 22 commits October 21, 2024 15:15

Design changes

1e0c5b2

removed the width of the text

c2de883

design changes

0b7cb00

prettier

82f3734

design changes

11950e5

text size change

5442a78

prettier

d192db7

design changes

f6c3fce

Changed alt text colour to white

441e0c7

Design changes

908ae5c

Design changes

2e34ee2

design changes

3ef028c

added a bit of margin to the image

57e978f

removed scrollbar

d4da90f

design changes

3feecfa

changed button texts

702542e

design changes

485b111

design changes

12071ed

Design changes to the sign in page

0400f6f

align icon

e9c3756

added border radius

dbec266

Let the user enter any email in forgot password page

c37a8b1

crisnicandrei force-pushed the PER-9491-new-sign-in branch from efe6f53 to c37a8b1 Compare October 21, 2024 12:17

fix tests

c81026f

meisekimiu requested review from emlesienk and removed request for emlesienk October 21, 2024 14:31

meisekimiu approved these changes Oct 21, 2024

View reviewed changes

crisnicandrei merged commit e01a472 into main Oct 21, 2024
4 checks passed

crisnicandrei deleted the PER-9491-new-sign-in branch October 21, 2024 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Per 9491 new sign in #366

Per 9491 new sign in #366

crisnicandrei commented Mar 5, 2024

codecov-commenter commented Mar 5, 2024 •

edited by codecov bot

Loading

meisekimiu left a comment

meisekimiu Mar 13, 2024

meisekimiu Mar 13, 2024

meisekimiu Mar 13, 2024

meisekimiu Mar 13, 2024

meisekimiu Mar 13, 2024

meisekimiu commented Mar 13, 2024

crisnicandrei commented Mar 14, 2024

meisekimiu commented Mar 15, 2024

meisekimiu left a comment

meisekimiu Mar 15, 2024

crisnicandrei commented Mar 15, 2024 •

edited

Loading

meisekimiu left a comment

meisekimiu Mar 27, 2024

meisekimiu Mar 27, 2024

meisekimiu Mar 27, 2024

meisekimiu Mar 27, 2024

crisnicandrei commented Mar 28, 2024

cecilia-donnelly commented May 2, 2024

crisnicandrei commented May 8, 2024

cecilia-donnelly commented Jul 16, 2024

meisekimiu left a comment

meisekimiu Jul 22, 2024

crisnicandrei Jul 23, 2024

meisekimiu Jul 22, 2024

emlesienk left a comment

k8lyn6 commented Jul 23, 2024

crisnicandrei commented Jul 23, 2024

crisnicandrei commented Jul 23, 2024


		import { LogoComponent } from '@auth/components/logo/logo.component';

		xdescribe('MfaComponent', () => {

	xdescribe('MfaComponent', () => {
	describe('MfaComponent', () => {

Per 9491 new sign in #366

Per 9491 new sign in #366

Conversation

crisnicandrei commented Mar 5, 2024

codecov-commenter commented Mar 5, 2024 • edited by codecov bot Loading

Codecov Report

meisekimiu 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

meisekimiu commented Mar 13, 2024

crisnicandrei commented Mar 14, 2024

meisekimiu commented Mar 15, 2024

meisekimiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisnicandrei commented Mar 15, 2024 • edited Loading

meisekimiu 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

crisnicandrei commented Mar 28, 2024

cecilia-donnelly commented May 2, 2024

crisnicandrei commented May 8, 2024

cecilia-donnelly commented Jul 16, 2024

meisekimiu 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

emlesienk left a comment

Choose a reason for hiding this comment

k8lyn6 commented Jul 23, 2024

crisnicandrei commented Jul 23, 2024

crisnicandrei commented Jul 23, 2024

codecov-commenter commented Mar 5, 2024 •

edited by codecov bot

Loading

crisnicandrei commented Mar 15, 2024 •

edited

Loading