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

Add password requirements to login page #103

Merged
merged 11 commits into from
Nov 18, 2023
Merged

Add password requirements to login page #103

merged 11 commits into from
Nov 18, 2023

Conversation

jaggederest
Copy link
Contributor

closes #90

Would appreciate some feedback on placement and styling - wanted to fit it into the Label element but thought it might be better after the form field

Copy link

changeset-bot bot commented Nov 13, 2023

⚠️ No Changeset found

Latest commit: 3c22f8d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@wrn14897
Copy link
Contributor

closes #90

Would appreciate some feedback on placement and styling - wanted to fit it into the Label element but thought it might be better after the form field

Thanks for fixing the issue. Do you mind sharing the screenshot ?

@jaggederest
Copy link
Contributor Author

closes #90
Would appreciate some feedback on placement and styling - wanted to fit it into the Label element but thought it might be better after the form field

Thanks for fixing the issue. Do you mind sharing the screenshot ?

Screenshot 2023-11-13 at 16 54 55

@wrn14897
Copy link
Contributor

closes #90
Would appreciate some feedback on placement and styling - wanted to fit it into the Label element but thought it might be better after the form field

Thanks for fixing the issue. Do you mind sharing the screenshot ?

Screenshot 2023-11-13 at 16 54 55

I think it would be cleaner if we have a list of policies underneath. Ideally policy checks automatically. But for now I think we can just leave it as a normal list.

Screenshot 2023-11-13 at 6 36 43 PM

@jaggederest
Copy link
Contributor Author

Nice, will do. We'll see how I get along with the checking-as-satisfied section, but at the very least a static list.

@jaggederest
Copy link
Contributor Author

Kind of a funky SVG but does the thing:

Screenshot 2023-11-14 at 00 34 45

Not sure why the useEffect call is getting {password: 'something'} instead of a raw string.

@jaggederest
Copy link
Contributor Author

New look with bootstrap SVGs:

Screenshot 2023-11-14 at 14 28 07

MikeShi42
MikeShi42 previously approved these changes Nov 17, 2023
Copy link
Contributor

@MikeShi42 MikeShi42 left a comment

Choose a reason for hiding this comment

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

final few nits I'd love to see if we can squeeze in - but things look great otherwise :) like the nice touch of the confirm password label being dynamic as well

packages/app/src/AuthPage.tsx Outdated Show resolved Hide resolved
packages/app/src/PasswordCheck.tsx Outdated Show resolved Hide resolved
packages/app/src/AuthPage.tsx Outdated Show resolved Hide resolved
@jaggederest
Copy link
Contributor Author

Local version is failing with

./src/useConfirm.tsx:2:0
Module not found: Can't resolve 'jotai'
  1 | import * as React from 'react';
> 2 | import { atom, useAtomValue, useSetAtom } from 'jotai';
  3 | import Modal from 'react-bootstrap/Modal';
  4 | import Button from 'react-bootstrap/Button';
  5 | 

Import trace for requested module:
./pages/_app.tsx

https://nextjs.org/docs/messages/module-not-found

Which is a weird one because it's installed and worked previously 🤷

Anyway, no screenshot from this round for that reason but the changes should be in place

Copy link
Contributor

@MikeShi42 MikeShi42 left a comment

Choose a reason for hiding this comment

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

looks great!

image

@MikeShi42
Copy link
Contributor

@jaggederest you might need to run your setup with --build to rebuild the image to include the new packages/setup. Lmk if that doesn't work for you though

@kodiakhq kodiakhq bot merged commit 7eebda6 into hyperdxio:main Nov 18, 2023
3 checks passed
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.

[HDX-135] Add password requirements into registration UI
4 participants