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.

Sign up for GitHub

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

SCC-4236: Username for My Account 2.0 #397

Merged
merged 28 commits into from
Nov 26, 2024
Merged

SCC-4236: Username for My Account 2.0 #397

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
766b558
Start of username
7emansell Nov 15, 2024
7e67c6e
Pulling out status banner
7emansell Nov 19, 2024
47219eb
Start of username validation api route
7emansell Nov 19, 2024
cd54635
Removing discovery links from config and updating nyplApiClient
7emansell Nov 19, 2024
f116cf8
Username form and cleanup in other files
7emansell Nov 19, 2024
a6f7684
Tests (need more once deletion is confirmed)
7emansell Nov 20, 2024
e52407e
Clean up
7emansell Nov 20, 2024
673d296
One more test
7emansell Nov 20, 2024
bc995e2
Typooo
7emansell Nov 21, 2024
f1a87f8
Moving UsernameForm into list
7emansell Nov 22, 2024
ee979fc
Typo/import clean up
7emansell Nov 22, 2024
28e698d
Simplifying ternaries
7emansell Nov 22, 2024
303cf48
Removing ternaries and fixing alignment
7emansell Nov 25, 2024
d240835
Removing DISCOVERY_API_NAME
7emansell Nov 25, 2024
8ec5985
Removing more ternary
7emansell Nov 25, 2024
4a5e036
Styling updates
7emansell Nov 25, 2024
d78b943
Tweaking error handling on helper
7emansell Nov 25, 2024
013d510
Changing deletion strategy
7emansell Nov 25, 2024
b17b5a1
Adding deletion test and defining tempInput null meaning
7emansell Nov 25, 2024
6570b9f
Commenting
7emansell Nov 25, 2024
0bcf57d
More clean up
7emansell Nov 25, 2024
bc4f37e
Variable name updates
7emansell Nov 25, 2024
63aa14e
More styling tweaks
7emansell Nov 25, 2024
f1e65a6
styling
7emansell Nov 25, 2024
2863e93
Starting to fix helper error text
7emansell Nov 26, 2024
5e720be
using ui.link.primary where possible
7emansell Nov 26, 2024
3b317c3
more changes
7emansell Nov 26, 2024
e7bf689
Tests corrected
7emansell Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pages/api/account/username/[id].ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export default async function handler(
const patronId = req.query.id as string
const { username } = req.body
/** We check that the patron cookie matches the patron id in the request,
* i.e.,the logged in user is updating their own useername. */
* i.e.,the logged in user is updating their own username. */
if (patronId == cookiePatronId) {
const response = await updateUsername(patronId, username)
responseStatus = response.status
Expand Down
62 changes: 36 additions & 26 deletions src/components/MyAccount/NewSettings/StatusBanner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,35 +7,45 @@ type StatusBannerProps = {
statusMessage: string
}

const successContent = (
<Text marginBottom={0} color={"ui.black !important"}>
7emansell marked this conversation as resolved.
Show resolved Hide resolved
Your changes were saved.
</Text>
)

const generalFailureContent = (
<Text marginBottom={0} color={"ui.black !important"}>
Your changes were not saved.
</Text>
)

const specificFailureContent = (statusMessage: string) => {
return (
<Text marginBottom={0} color={"ui.black !important"}>
{statusMessage} Please try again or{" "}
<Link
sx={{
color: "ui.link.primary !important",
textDecorationColor: "ui.link.primary !important",
Copy link
Member

Choose a reason for hiding this comment

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

Two things:

  • I'm not sure this works, or rather, this should not work. I have not successfully added a design token value with !important. So this should be updated to "var(--nypl-colors-ui-link-primary) !important".
  • The above point should be resolved if you use the default style. Links in the negative Banner are meant to be red:
Screenshot 2024-11-22 at 5 43 52 PM

Are they blue in the designs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are blue in the designs and this does work but i'll change it

textDecoration: "underline",
}}
href="https://www.nypl.org/get-help/contact-us"
>
contact us
</Link>{" "}
for assistance.
</Text>
)
}

export const StatusBanner = ({ status, statusMessage }: StatusBannerProps) => {
const bannerContent = (
<div style={{ alignItems: "center" }}>
{status === "failure" ? (
statusMessage !== "" ? (
<Text marginBottom={0} color={"ui.black !important"}>
{statusMessage} Please try again or{" "}
<Link
sx={{
color: "ui.link.primary !important",
textDecorationColor: "ui.link.primary !important",
textDecoration: "underline",
}}
href="https://www.nypl.org/get-help/contact-us"
>
contact us
</Link>{" "}
for assistance.
</Text>
) : (
<Text marginBottom={0} color={"ui.black !important"}>
Your changes were not saved.
</Text>
)
) : (
<Text marginBottom={0} color={"ui.black !important"}>
Your changes were saved.
</Text>
)}
{status === "failure"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely better, but I'd really like to have this nested ternary replaced. Its ok if you end up with a conditional that seems redundant or repetitive, ie if failure && statusMessage, if failure && !statusMessage and if !failure. the conditional logic doesn't have to be inline of the return statement. in fact, it might have to come out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay ternary gone but I'd like to register my dissent... I think this makes it less readable both here and in the username form

? statusMessage !== ""
? specificFailureContent(statusMessage)
: generalFailureContent
: successContent}
</div>
)

Expand Down
3 changes: 0 additions & 3 deletions src/components/MyAccount/NewSettings/UsernameForm.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ describe("username form", () => {
setUsernameStatus: jest.fn(),
setUsernameStatusMessage: jest.fn(),
}
const accountFetchSpy = jest.fn()

beforeEach(() => {
jest.clearAllMocks()
Expand All @@ -26,7 +25,6 @@ describe("username form", () => {

const component = (
<PatronDataProvider
testSpy={accountFetchSpy}
value={{
patron: processedPatron,
pickupLocations: filteredPickupLocations,
Expand All @@ -41,7 +39,6 @@ describe("username form", () => {

const noUsernameComponent = (
<PatronDataProvider
testSpy={accountFetchSpy}
value={{
patron: emptyPatron,
pickupLocations: filteredPickupLocations,
Expand Down
175 changes: 90 additions & 85 deletions src/components/MyAccount/NewSettings/UsernameForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,101 +101,106 @@ const UsernameForm = ({ patron, usernameState }: UsernameFormProps) => {
}
}

const editUsernameField = (
<>
<Flex flexDir="row" alignItems="flex-start">
<TextInput
sx={{
width: { base: "87%", md: "300px" },
display: "flex",
flexDirection: "column-reverse",
label: {
fontWeight: "400",
color: error ? "ui.error.primary" : "ui.black",
Copy link
Member

Choose a reason for hiding this comment

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

This should happen automatically when isInvalid is true. Or is this because the additional !validateUsername check below affects the text color?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because the label and the invalid text are the same copy, and render in the same place (below the field). Since that's not how this component expects to work, I'm manually making the label text become the invalid text when needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use helperText here instead of labelText? It looks like the helper text is automatically with the correct styling for this.

},
}}
value={tempInput || ""}
id="username-input"
labelText="Must be 5-15 characters and use only letters (a-z) and numbers (0-9)"
Copy link
Member

Choose a reason for hiding this comment

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

The space between the input and label seems too small, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, j added more styling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's like a mix of the label and helper text on this component I'm not sure how design arrived at this

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is a default that makes sense functionally (like the helper text) coming from reservoir that deviates from the designs in a small way, can you ask Apoorva if it's alright to use the default instead of overriding?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I see what's going on. I thought this was the helperErrorText because it was below the input field but you're reversing the order on line 117. I don't understand why this was done.

You can try target the helperErrorText for styling purposes. Can you try having the text value in the helper text prop and invalid text prop? Then you can update the color with the isInvalid prop.

My concern is that a screenreader would read the "Must be 5-15 characters and use only letters (a-z) and numbers (0-9)" text but that's not necessarily clear that it's for the username. So a hidden "username" label, I think, is better. Let's chat about this.

showLabel
isInvalid={error && !validateUsername(tempInput)}
showHelperInvalidText={false}
onChange={handleInputChange}
isClearable
isClearableCallback={() => setError(true)}
/>
<Button
aria-label="Remove username"
buttonType="text"
id="remove-username-btn"
onClick={() => {
setTempInput(null)
setError(false)
}}
>
{" "}
<Icon name="actionDelete" size="large" />
</Button>
</Flex>
<Banner
sx={{ marginTop: "xs", width: "fill" }}
content="If you delete your username, you will have to use your barcode to log in to your account in the future."
type="warning"
/>
</>
)

const notEditingView = (
<Flex alignItems="center">
{input !== "" ? (
<>
<Text size="body1" sx={{ marginBottom: 0 }}>
{input}
</Text>
<EditButton
buttonId="edit-username-button"
onClick={() => setIsEditing(true)}
/>
</>
) : (
<AddButton
label="+ Add username"
onClick={() => {
setIsEditing(true)
setTempInput("")
}}
/>
)}
</Flex>
)

const editingView = (
<Flex
flexDir="column"
marginLeft={{ base: "l", lg: "unset" }}
marginTop={{ base: "xs", lg: "unset" }}
maxWidth={{ base: "600px", md: "320px" }}
>
{tempInput !== null ? (
editUsernameField
) : (
<AddButton
label="+ Add username"
onClick={() => {
setTempInput("")
}}
/>
)}
</Flex>
)

return (
<Flex
flexDir={{ base: "column", lg: "row" }}
alignItems="flex-start"
width="100%"
>
<Flex gap="xs" marginRight="140px" paddingTop="xs" alignItems="center">
<Icon name="actionIdentity" size="large" />
<Text size="body1" sx={{ fontWeight: "500", marginBottom: 0 }}>
Username
</Text>
</Flex>

{isLoading ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar request here. Can you replace this nested ternary with a conditional outside of the return statement so it's a easier to scan which components get rendered when?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again lmk if this is better/ more readable

Copy link
Contributor

Choose a reason for hiding this comment

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

Same note here - the extraction is good, but the nested ternary itself is hard to read. Can you replace it?

<SkeletonLoader contentSize={2} showImage={false} headingSize={0} />
) : isEditing ? (
<Flex
flexDir="column"
marginLeft={{ base: "l", lg: "unset" }}
marginTop={{ base: "xs", lg: "unset" }}
maxWidth={{ base: "600px", md: "320px" }}
>
{tempInput !== null ? (
<>
<Flex flexDir="row" alignItems="flex-start">
<TextInput
sx={{
width: { base: "87%", md: "300px" },
display: "flex",
flexDirection: "column-reverse",
label: {
fontWeight: "400",
color: error ? "ui.error.primary" : "ui.black",
},
}}
value={tempInput || ""}
id="username-input"
labelText="Must be 5-15 characters and use only letters (a-z) and numbers (0-9)"
showLabel
isInvalid={error && !validateUsername(tempInput)}
showHelperInvalidText={false}
onChange={handleInputChange}
isClearable
isClearableCallback={() => setError(true)}
/>
<Button
aria-label="Remove username"
buttonType="text"
id="remove-username-btn"
onClick={() => {
setTempInput(null)
setError(false)
}}
>
{" "}
<Icon name="actionDelete" size="large" />
</Button>
</Flex>
<Banner
sx={{ marginTop: "xs", width: "fill" }}
content="If you delete your username, you will have to use your barcode to log in to your account in the future."
type="warning"
/>
</>
) : (
<AddButton
label="+ Add username"
onClick={() => {
setTempInput("")
}}
/>
)}
</Flex>
editingView
) : (
<Flex alignItems="center">
{input !== "" ? (
<>
<Text size="body1" sx={{ marginBottom: 0 }}>
{input}
</Text>
<EditButton
buttonId="edit-username-button"
onClick={() => setIsEditing(true)}
/>
</>
) : (
<AddButton
label="+ Add username"
onClick={() => {
setIsEditing(true)
setTempInput("")
}}
/>
)}
</Flex>
notEditingView
)}

{isEditing && (
Expand Down
21 changes: 8 additions & 13 deletions src/components/MyAccount/ProfileHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ const ProfileHeader = ({ patron }: { patron: Patron }) => {
const profileData = (
[
{ icon: "actionIdentityFilled", term: "Name", description: patron.name },
{
icon: "actionIdentity",
term: "Username",
description: (
<UsernameForm patron={patron} usernameState={usernameState} />
),
},
{
7emansell marked this conversation as resolved.
Show resolved Hide resolved
icon: "actionPayment",
term: "Card number",
Expand Down Expand Up @@ -90,19 +97,7 @@ const ProfileHeader = ({ patron }: { patron: Patron }) => {
h2: { border: "none", paddingTop: 0 },
}}
>
{profileData[0]}
</List>
<UsernameForm patron={patron} usernameState={usernameState} />
<List
className={styles.myAccountList}
id="my-account-profile-header-2"
type="dl"
sx={{
border: "none",
marginBottom: "xxl",
}}
>
{profileData.slice(1)}
{profileData}
</List>
</>
)
Expand Down