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

Per 9756 update archive type onboarding screen #439

Merged
merged 9 commits into from
Sep 17, 2024

Conversation

crisnicandrei
Copy link
Contributor

Design changes for the archive type select screen.

@yeslikesolo I am missing some texts for the "Other" and "I'm not sure yet" so we keep the header text format please!

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

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

Project coverage is 42.38%. Comparing base (1b81675) to head (e059254).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...create-new-archive/create-new-archive.component.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #439      +/-   ##
==========================================
- Coverage   42.40%   42.38%   -0.03%     
==========================================
  Files         346      346              
  Lines       10836    10837       +1     
  Branches     1768     1769       +1     
==========================================
- Hits         4595     4593       -2     
- Misses       6217     6220       +3     
  Partials       24       24              

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

Copy link
Member

@meisekimiu meisekimiu left a comment

Choose a reason for hiding this comment

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

I've added some small bits of initial feedback before we pass it to QA.

@@ -9,7 +10,7 @@

border-radius: 12px;
border: 1px solid rgba(255, 255, 255, 0.16);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
border: 1px solid rgba(255, 255, 255, 0.16);
border: none;

I think one of the things requested in the ticket is to remove a weird orange line that appears near the component. It seems like this change gets rid of that.

@@ -5,8 +5,8 @@
background: #fff;
border-radius: 12px 12px 0px 0px;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
border-radius: 12px 12px 0px 0px;
border-radius: 12px;

The design for this dialog actually changed at some point after it was initially implemented. It is now not flush with the bottom of the screen, so I believe we have a consistent border radius now.

Comment on lines +25 to 30
if (this.tag) {
this.buttonText = generateElementText(
this.tag,
archiveOptionsWithArticle,
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Codecov is pointing out that this is missing coverage. It would be nice to write a test that renders the component with the tag input provided.

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-09-05 at 5 16 07 PM
  • Please decrease the width of H1 component Create your first archive, to half its size. It should stack or be wrapped text, aligned left.
  • Increase bold for "Archive" in h1 text for create new archive component
Screenshot 2024-09-05 at 5 33 57 PM
  • bold archive type names: Personal, An Individual, Family, etc.

@crisnicandrei
Copy link
Contributor Author

@yeslikesolo can't do much bolder, this is tthe only bold font that i have :(

@k8lyn6
Copy link

k8lyn6 commented Sep 13, 2024

@crisnicandrei could you confirm what font you're using for onboarding? The bold/not bold has been hard to see across onboarding and I just want to check we're using the right font. If the font is correct, could you coordinate with Tibi about why it's not looking like the design?

@yeslikesolo once Andrei has checked that, could you re-review this?

@crisnicandrei
Copy link
Contributor Author

@k8lyn6 @yeslikesolo I think I got it right this time. Can you please recheck? It was Usual, but I had to use the extra bold instead

@crisnicandrei crisnicandrei force-pushed the PER-9756-update-archive-type-onboarding-screen branch from 8dfb34d to e059254 Compare September 17, 2024 19:06
@crisnicandrei crisnicandrei merged commit e58de3d into main Sep 17, 2024
4 checks passed
@crisnicandrei crisnicandrei deleted the PER-9756-update-archive-type-onboarding-screen branch September 17, 2024 19:11
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.

4 participants