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

List all accepted archives at end of Glam onboarding #491

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

meisekimiu
Copy link
Member

@meisekimiu meisekimiu commented Nov 19, 2024

Right now the final congratulations / "finalize" screen lists a user's created archive. If a user has archive invites they can accept multiple archives during onboarding but only one is showing and it is listed as an owner regardless of the access role of the archive. Update this congrats screen to handle multiple archives and list out all their access roles correctly.

This PR involves the creation of a new OnboardingService which mainly acts as a place to store "registered archives" and then get that final list of archives in the end. This is a better pattern for sharing information between components with complex flows compared to the Input/Output system we're using now, but I also think that the onboarding flow has complex enough behavior to warrant some of that behavior moving into a dedicated service anyway. I didn't want this 1 point ticket to involve a complete refactoring of glam onboarding logic, so this service is pretty bare right now. As we continue to improve the onboarding flow, I hope we can continue to move things into this service.

Steps to test:

  1. Run through the glam onboarding flow and verify that when you create an archive it still shows up properly.
  2. Run through the glam onboarding flow with multiple archive invitations, preferably with access roles besides owner. Verify that at the end of the process all archives are listed with correct access roles.

This can be tested through the onboarding Storybook demo by running npm run storybook.

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 42.75%. Comparing base (e7d0321) to head (deaae15).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/app/onboarding/onboarding.module.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #491      +/-   ##
==========================================
+ Coverage   42.71%   42.75%   +0.04%     
==========================================
  Files         359      360       +1     
  Lines       10974    10986      +12     
  Branches     1793     1794       +1     
==========================================
+ Hits         4687     4697      +10     
- Misses       6128     6131       +3     
+ Partials      159      158       -1     

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

@meisekimiu
Copy link
Member Author

The 1 line of missing coverage is an angular module constructor, which I think is fine to leave uncovered.

<div>
<img
src="/assets/svg/onboarding/default-archive.svg"
alt="Custom Archive Icon"
Copy link
Member

Choose a reason for hiding this comment

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

This didn't change, but is this useful alt text?

Copy link
Member Author

Choose a reason for hiding this comment

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

It probably isn't and it might be better to use an aria-labeledby attribute instead here!

Copy link

@yeslikesolo yeslikesolo left a comment

Choose a reason for hiding this comment

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

Step 1 Testing Comments:
I am unable to see my newly created archive on the screen. Please see image below. After I hit "next" on this screen, it shows the updated screen with my archive listed.
Screenshot 2024-11-21 at 12 00 17 PM

Step 2 Testing Comments:

  • Move paragraph text "You've been invited to..." to right hand side, above archive list.
  • "accept" button is not functional
Screen.Recording.2024-11-21.at.12.12.03.PM.mov

@k8lyn6
Copy link

k8lyn6 commented Dec 2, 2024

@meisekimiu is there anything you are waiting on from me or @yeslikesolo for this? Just wanted to check!

The onboarding flow(s) used in the app tend to involve a lot of small
components passing data to each other. This communication can be simple
and lightweight, but as components and flows grow in complexity this
becomes harder to manage. Fortunately we can use Angular services as a
way to refactor this code. Create the OnboardingService that stores
created/accepted archives so we can show them back to the user in the
final screen without having to differentiate between created and
accepted archives.

While it's unclear if just this refactoring requires a whole service,
there have been many occurences where consolidating onboarding code into
a central service would have been useful in the past few months. So
while the implementation of this service leaves a lot to be desired,
this is at least a good first step towards moving more of the onboarding
flow's behavior into this service instead of components. This in turn
would allow us to redesign the onboarding flow in the future without
worrying about damaging fundamental onboarding behavior.

PER-9891: Final glam onboarding screen should list all accepted archives
Get finalized archives from the OnboardingService to display in the
`pr-finalize-archive-creation-screen` component and update the component
to iterate over a list of archives instead of just displaying the name.
This also means updating the component so access role is not hardcoded
to list "OWNER".

PER-9891: Final glam onboarding screen should list all accepted archives
@meisekimiu meisekimiu force-pushed the glam-onboarding-all-accepted-archives branch from 4521a87 to 2d94f04 Compare December 3, 2024 14:05
@meisekimiu
Copy link
Member Author

Screenshot from 2024-12-03 10-59-36
Should the text be set up like this? In the design it's on the left side of the screen so I'm not sure how the spacing should look. @yeslikesolo / @k8lyn6
(I've also fixed the unreadable small text on this screen)

As for the accept button issue, I'm not sure that's connected to this PR and it might be a network or other strange environment issue. Can we try testing that again? It seems to work when I test on a newly reset local development environment as well as on Staging.

@k8lyn6
Copy link

k8lyn6 commented Dec 3, 2024

Hey @meisekimiu , my apologies but I think the text on the "Welcome with archive invitations" page should go wherever it indicates in the design. If it's on the left side, then we should keep it there. cc @yeslikesolo can you test whether the accept button is still not functional for you?

@meisekimiu meisekimiu force-pushed the glam-onboarding-all-accepted-archives branch from e088d61 to 44ad466 Compare December 3, 2024 19:16
@yeslikesolo
Copy link

@k8lyn6 @meisekimiu hmm that's weird, maybe I'm looking at an outdated design file? Because its on the right side for me. Either way, whichever is fine! I was following the design file that I thought was the source of truth.

@meisekimiu
Copy link
Member Author

Screenshot from 2024-12-04 10-30-14
This is a screenshot of the design I checked. There is text is on the right side on some other screens, but as far as I know it's on the left on this specific screen.

Copy link

@yeslikesolo yeslikesolo left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-12-04 at 11 26 38 AM

I know @meisekimiu flagged she already fixed the small text - just wanted to note it here too!

Screen.Recording.2024-12-04.at.11.29.32.AM.mov

Accept button worked when I first hit accept. I then hit "create new archive", decided to go back to previous screen, and now the accept button is no longer working. Please see screen recording above.

If a user hits the "approve" button, use the new OnboardingService to
remember this if the user hits back during the process. Also, adjust how
archives are added to the service so that a user can accept archives AND
create a new archive, and all archives are present at the end of the
list.

PER-9891: Final glam onboarding screen should list all accepted archives
Copy link

@yeslikesolo yeslikesolo left a comment

Choose a reason for hiding this comment

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

Everything is fixed, thanks @meisekimiu !

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.

5 participants