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

feat: TUP-704 simplify TAM links UI #431

Merged
merged 21 commits into from
Mar 7, 2024

Conversation

wesleyboar
Copy link
Member

@wesleyboar wesleyboar commented Feb 22, 2024

Overview

Reduce complexity of TAM links UI. Add more useful context.

Status

  • Let TAM review.
  • Self, either clean up excess diff or overhaul markup completely.

Related

Changes

  • deleted css & markup
  • changed css & markup

Testing

  1. Visit "Manage Account" page.
  2. Feel out the UI for bugs (responsiveness, spacing, font, et cetera).
  3. Confirm designer approval.

UI

Before Before (More) After
before before - scrolled option 4A - tweak 1
Archived
we can do LINKS we can do BUTTONS
option 2C option 3B

@wesleyboar wesleyboar force-pushed the feat/TUP-704-simplify-tam-links-ui branch from 08ddb96 to 6b5314e Compare February 22, 2024 00:28
@wesleyboar wesleyboar force-pushed the feat/TUP-704-simplify-tam-links-ui branch from 6150547 to e6384cf Compare March 5, 2024 22:15
@wesleyboar wesleyboar marked this pull request as ready for review March 5, 2024 22:26
Comment on lines -24 to -29
.account-body > section {
display: flex;
flex-direction: column;
gap: 20px;
height: 100%;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Simplifies layout. This had arranged blocks vertical and added space between them. Yes. But blocks by default arrange vertically. And spacing could differ and is unexpected to exist in one location for regularly-stacked content, so add spacing around elements as needed.

Comment on lines -44 to -46
font-size: 1rem;
font-style: italic;
color: #484848;
Copy link
Member Author

@wesleyboar wesleyboar Mar 6, 2024

Choose a reason for hiding this comment

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

Was redundant or undesirable. Upon review, designer declined italics.

</>
);

const ManageDNs = () => (
Copy link
Member Author

@wesleyboar wesleyboar Mar 6, 2024

Choose a reason for hiding this comment

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

Beware, the position of items in diff is misleading. The manageDNs is shown further down. The managePassword is removed.

Copy link
Collaborator

@jarosenb jarosenb left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@R-Tomas-Gonzalez R-Tomas-Gonzalez left a comment

Choose a reason for hiding this comment

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

Looks good. Only thing I wish for cleanliness of markup, I wish each little subsection was in it's own div container. But no need to fix for this. Extra worry we need not.

APPROVED.

Screenshot 2024-03-06 at 4 57 17 PM

@wesleyboar
Copy link
Member Author

Thanks, @R-Tomas-Gonzalez. I agree fully. I actually cleaned up and then de-cleaned the markup in prior commits. I'm trying to train myself to not combine refactors with other tasks… it's uncomfortable, but I convinced myself there was a benefit.

@wesleyboar wesleyboar merged commit f06485a into main Mar 7, 2024
1 check passed
@wesleyboar wesleyboar deleted the feat/TUP-704-simplify-tam-links-ui branch March 7, 2024 18:38
@wesleyboar
Copy link
Member Author

@R-Tomas-Gonzalez, as available, let me know what you think of the markup (and style) changes in #439.

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.

None yet

3 participants