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

[Feature] Account settings page #9971

Merged
merged 54 commits into from
May 1, 2024
Merged

[Feature] Account settings page #9971

merged 54 commits into from
May 1, 2024

Conversation

yonikid15
Copy link
Contributor

@yonikid15 yonikid15 commented Apr 3, 2024

🤖 Resolves #9556

👋 Introduction

  • Adds account settings page

🕵️ Details

Add any additional details that could assist with reviewing or testing this PR.

🧪 Testing

Assist reviewers with steps they can take to test that the PR does what it says it does.

  1. Go to http://localhost:8000/en/applicant/settings
  2. Ensure page looks and functions correctly

mnigh
mnigh previously requested changes Apr 3, 2024
Copy link
Contributor

@mnigh mnigh left a comment

Choose a reason for hiding this comment

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

Not sure if this PR is still being worked on, but some initial suggestions and questions.

  1. GCKey should not have a space, REF: https://www.btb.termiumplus.gc.ca/tpv2alpha/alpha-eng.html?lang=eng&i=1&srchtxt=GCKEY&codom2nd_wet=1#resultrecs

  2. NotificationSettings section, according to the linked issue, should be behind a feature flag (I would guess FEATURE_NOTIFICATIONS)

  3. Notification settings are not being saved in the database

@yonikid15
Copy link
Contributor Author

Not sure if this PR is still being worked on, but some initial suggestions and questions.

  1. GCKey should not have a space, REF: https://www.btb.termiumplus.gc.ca/tpv2alpha/alpha-eng.html?lang=eng&i=1&srchtxt=GCKEY&codom2nd_wet=1#resultrecs

I just followed the design 😭 I'll make the change

  1. NotificationSettings section, according to the linked issue, should be behind a feature flag (I would guess FEATURE_NOTIFICATIONS)

Thanks I missed that!

  1. Notification settings are not being saved in the database

I think I fixed that now? Were you referring to just the job alert email setting or all the settings aren't saving?

@yonikid15 yonikid15 requested a review from mnigh April 4, 2024 18:00
@yonikid15 yonikid15 requested a review from mnigh April 8, 2024 17:48
@yonikid15 yonikid15 added blocked: question Further information is requested. blocked: copy Blocked by missing copy or translations labels Apr 10, 2024
apps/web/src/lang/fr.json Outdated Show resolved Hide resolved
@esizer
Copy link
Member

esizer commented Apr 30, 2024

I notice two things.

  1. Mobile layout could possibly use some work
  2. I can't seem to actually click the checkboxes, just next to them 😕

screenshot_30-Apr-2024_13-37-1714498630

2024-04-30_13-42-22.mp4

@yonikid15
Copy link
Contributor Author

yonikid15 commented Apr 30, 2024

I notice two things.

1. Mobile layout could possibly use some work

2. I can't seem to actually click the checkboxes, just next to them 😕

@esizer
Ya I'll try adding padding to the mobile layout.
For, clicking the checkboxes are you sure you can't click them? I'm seeing the same effect across the app where the pointer is only working around the checkboxes, but I can definitely click them.

Personal.information._.GC.Digital.Talent.Firefox.Developer.Edition.2024-04-30.15-31-38.mp4

@yonikid15 yonikid15 requested a review from mnigh April 30, 2024 19:47
@esizer
Copy link
Member

esizer commented May 1, 2024

For, clicking the checkboxes are you sure you can't click them? I'm seeing the same effect across the app where the pointer is only working around the checkboxes, but I can definitely click them.

I can in chrome but in firefox it is kinda tricky and not a great experience. This is an issue with the base component but the removal of a label exacerbates the issue. Also, without the label, these checkboxes are not great when you are using a zoom tool. As you can see, I just see a checkbox with no context when a zoom tool is enabled which is a less than great experience.

screenshot_01-May-2024_09-09-1714568992

@yonikid15
Copy link
Contributor Author

yonikid15 commented May 1, 2024

For, clicking the checkboxes are you sure you can't click them? I'm seeing the same effect across the app where the pointer is only working around the checkboxes, but I can definitely click them.

I can in chrome but in firefox it is kinda tricky and not a great experience. This is an issue with the base component but the removal of a label exacerbates the issue. Also, without the label, these checkboxes are not great when you are using a zoom tool. As you can see, I just see a checkbox with no context when a zoom tool is enabled which is a less than great experience.

Hmm, ya that is a problem. I can't really do anything about that with this design. @substrae Can we change the design to add the labels next to the checkbox instead?

@tristan-orourke
Copy link
Member

If there are issues with the base checkbox component (pointer, zooming) let's make a new ticket and keep them out-of-scope for this PR.

@tristan-orourke
Copy link
Member

Also, I'm not particularly concerned about the zooming. If we're using columns correctly now, this is basically a small table, where it's natural that you might have to pan a bit to git the full context of any given cell.

@yonikid15
Copy link
Contributor Author

I got a new mobile design for the Notifications Section from @substrae
Design:
image (7)

I pushed a commit 5280bc5 that makes those changes. Please take a look and let me know if it's correct @tristan-orourke

Screenshot:

image

@yonikid15 yonikid15 enabled auto-merge May 1, 2024 18:45
@mnigh mnigh dismissed their stale review May 1, 2024 18:47

someone else approved

@yonikid15 yonikid15 added this pull request to the merge queue May 1, 2024
Merged via the queue into main with commit 7c20975 May 1, 2024
11 of 12 checks passed
@yonikid15 yonikid15 deleted the 9556-account-settings-page branch May 1, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ Account settings page creation
7 participants