-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[BEEEP] [PM-10117] Migrate index.html #10286
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #10286 +/- ##
=======================================
Coverage 32.78% 32.78%
=======================================
Files 2665 2665
Lines 81354 81354
Branches 15320 15320
=======================================
Hits 26668 26668
Misses 52647 52647
Partials 2039 2039 ☔ View full report in Codecov by Sentry. |
New Issues
|
…ndex # Conflicts: # apps/web/src/scss/tailwind.css
…ndex # Conflicts: # libs/auth/src/angular/anon-layout/anon-layout.component.html # libs/auth/src/angular/anon-layout/anon-layout.component.ts # libs/auth/src/angular/icons/index.ts
@@ -16,9 +16,9 @@ | |||
</head> | |||
|
|||
<body class="tw-min-h-screen !tw-min-w-0 tw-text-center tw-bg-background-alt tw-flex tw-flex-col"> | |||
<main class="tw-max-w-3xl tw-mx-auto tw-mb-8 tw-px-2"> | |||
<img src="images/logo.svg" width="200px" class="tw-py-16" alt="Bitwarden" /> | |||
<img class="new-logo-themed tw-m-8" alt="Bitwarden" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ I think all content is supposed to be in a landmark so I think it should be in main
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that, but I would need to move. the classes to a div to maintain the design.
@@ -5,7 +5,7 @@ | |||
<meta name="viewport" content="width=1010" /> | |||
<meta name="theme-color" content="#175DDC" /> | |||
|
|||
<title page-title>Bitwarden Web Vault</title> | |||
<title page-title>Bitwarden Web vault</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Why leave Web as title case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep it consistent to other places. The Angular apps uses lowercase.
/** | ||
* Logo, used both in loading and on "frontend" pages. | ||
*/ | ||
img.new-logo-themed { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Do we need the word "new" in this class name? (Only because eventually it won't be new, but we probably won't go update this class)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because logo-themed
is already taken. 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -2,7 +2,7 @@ | |||
class="tw-flex tw-min-h-screen tw-w-full tw-mx-auto tw-flex-col tw-gap-7 tw-bg-background-alt tw-px-8 tw-pb-4 tw-text-main" | |||
[ngClass]="{ 'tw-pt-0': decreaseTopPadding, 'tw-pt-8': !decreaseTopPadding }" | |||
> | |||
<bit-icon *ngIf="!hideLogo" [icon]="logo" class="tw-max-w-36"></bit-icon> | |||
<bit-icon *ngIf="!hideLogo" [icon]="logo" class="tw-w-[128px] [&>*]:tw-align-top"></bit-icon> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was needed to ensure consistency between the loading page and angular.
@rr-bw I rolledback the changes to annon-layout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to approve UI tests!
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-10117
📔 Objective
Migrate the
index.html
to use tailwind instead of bootstrap styling.📸 Screenshots
Screen.Recording.2024-07-26.at.14.00.16.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes