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.

Sign up for GitHub

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

Jump to bottom

Co-locate images with their usage site #1998

Merged
merged 3 commits into from
Jun 28, 2022
Merged

Co-locate images with their usage site #1998

merged 3 commits into from
Jun 28, 2022

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented May 25, 2022 •
edited
Loading

This was extracted from #1891.

This co-locates images with their site of use, where possible, rather than referring to them in ../../(etc)/static, which was done to ease the transition. This was requested in reviews of The Big One.

How to test: nothing should have changed.

  • l10n changes have been submitted to the l10n repository, if any.
  • I've added a unit test to test for potential regressions of this bug. - N/A
  • I've added or updated relevant docs in the docs/ directory.
  • All UI revisions follow the coding standards, and use Protocol tokens where applicable (see /static/scss/libs/protocol/css/includes/tokens/dist/index.scss).
  • Commits in this PR are minimal and have descriptive commit messages.

@Vinnl Vinnl requested a review from maxxcrawford May 25, 2022 13:52
@Vinnl Vinnl self-assigned this May 25, 2022
@Vinnl Vinnl mentioned this pull request May 25, 2022
Merged
5 tasks
@Vinnl Vinnl force-pushed the MPP-1575-remove-redundant-ui-images branch from 848a69f to 9385af1 Compare May 25, 2022 14:01
@Vinnl Vinnl changed the base branch from main to MPP-1575-remove-redundant-ui May 25, 2022 14:09
jwhitlock
jwhitlock reviewed May 26, 2022
View reviewed changes
Copy link
Member

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

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

I checked that wrapped emails continue to render with this change. It looks like the images were moved back in PR #1767, so this removes the duplicates in the original location.

@Vinnl Vinnl added the size:S about a day label May 31, 2022
maxxcrawford
maxxcrawford approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@maxxcrawford maxxcrawford left a comment

Choose a reason for hiding this comment

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

Thanks for breaking this out into two PRs! Good to go.

frontend/src/components/dashboard/Onboarding.module.scss
@@ -38,7 +38,7 @@

&::before {
content: counter(step-counter);
background-image: url("../../../../static/images/dashboard-onboarding/shield.svg");
background-image: url("./images/shield.svg");
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: This is sooooo much cleaner. 🙌

frontend/src/components/dashboard/PremiumOnboarding.module.scss
@@ -69,6 +69,10 @@
align-items: center;
gap: $spacing-xs;
padding-bottom: $spacing-xs;

svg {
color: $color-blue-50;
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Using currentColor will make this way more flexible.

frontend/src/components/dashboard/aliases/Alias.tsx
className={styles["copy-icon"]}
/>
<span className={styles["copy-icon"]}>
<CopyIcon alt="" />
Copy link
Contributor

Choose a reason for hiding this comment

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

question (nit, non-blocking): Should this have an entry in the alt tag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so, since it's contained within a button that already has an aria-label. (This means that instead of a button that just has the mask address followed by "click to copy", the whole thing is labelled something like "click to copy to clipboard", which is less confusing for a button.

frontend/src/components/dashboard/subdomain/ConfirmationModal.module.scss
img {
margin-right: $spacing-sm;
}
gap: $spacing-sm;
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: gap on the parent > margin on the child element forever.

frontend/src/components/dashboard/aliases/Alias.tsx
@@ -8,8 +8,7 @@ import {
useTooltipTrigger,
} from "react-aria";
import styles from "./Alias.module.scss";
import copyIcon from "../../../../../static/images/copy-to-clipboard.svg";
import arrowDownIcon from "../../../../../static/images/arrowhead.svg";
import { ArrowDownIcon, CopyIcon } from "../../Icons";
Copy link
Contributor

Choose a reason for hiding this comment

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

question (non-blocking): Should these also be in /public/images directory? (There's only four items in the icons directory)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are actually React components, rather than separate files (i.e. the full filename is Icons.tsx). The reason for that is that that allows us to inline the SVGs, which then allows us to use currentcolor, which is useful for symbolic items that have just a single colour that might differ depending on their context/hover state.

@Vinnl
Copy link
Collaborator Author

Vinnl commented Jun 9, 2022

Thanks for the review! I'll refrain from merging this until #1891 is merged, since it merges into that branch and therefore would become part of the PR anyway, removing the benefit of having them split out.

@Vinnl Vinnl force-pushed the MPP-1575-remove-redundant-ui branch from 64c57a3 to a35e9a4 Compare June 9, 2022 15:29
@Vinnl Vinnl added Review: XS Code review time: up to 30min and removed size:S about a day labels Jun 13, 2022
@Vinnl Vinnl force-pushed the MPP-1575-remove-redundant-ui branch from a35e9a4 to d4e9ee7 Compare June 17, 2022 09:10
@Vinnl Vinnl mentioned this pull request Jun 24, 2022
Merged
5 tasks
@Vinnl Vinnl force-pushed the MPP-1575-remove-redundant-ui branch from d4e9ee7 to b53f5af Compare June 27, 2022 11:26
Base automatically changed from MPP-1575-remove-redundant-ui to main June 28, 2022 11:04
@Vinnl Vinnl force-pushed the MPP-1575-remove-redundant-ui-images branch from 9385af1 to d539fbd Compare June 28, 2022 11:55
@Vinnl
Copy link
Collaborator Author

Vinnl commented Jun 28, 2022

Rebased on main. Quite a few conflicts, so I hope I didn't break anything 🤞

@netlify
Copy link

netlify bot commented Jun 28, 2022

❌ Deploy Preview for fx-relay-demo failed.

Name Link
🔨 Latest commit d539fbd
🔍 Latest deploy log https://app.netlify.com/sites/fx-relay-demo/deploys/62baec38820acd000a1c7d00

@Vinnl Vinnl force-pushed the MPP-1575-remove-redundant-ui-images branch from cd62dac to aaffc3a Compare June 28, 2022 12:18
@Vinnl Vinnl merged commit 02d920d into main Jun 28, 2022
@Vinnl Vinnl deleted the MPP-1575-remove-redundant-ui-images branch June 28, 2022 12:56
@Vinnl Vinnl mentioned this pull request Jun 29, 2022
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwhitlock jwhitlock jwhitlock left review comments

@maxxcrawford maxxcrawford maxxcrawford approved these changes

Assignees

@Vinnl Vinnl

Labels
Review: XS Code review time: up to 30min
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Vinnl @jwhitlock @maxxcrawford