Update "improve your measurement" section mobile design to match Figma #9832
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
aaemnnosttv
reviewed
Dec 6, 2024
| @media (min-width: $bp-tablet) { | ||
| margin-left: 42px; // margin-left = 32px switch width + 10px margin-left of label. | ||
| } | ||
| margin: 6px 0 0 42px; // margin-left = 32px switch width + 10px margin-left of label. |
There was a problem hiding this comment.
Can we use calc to do this instead of hard coding with a comment?
|
Build files for 06417ac have been deleted. |
techanvil
commented
Dec 9, 2024
There was a problem hiding this comment.
Thanks @benbowler, please see my comment about the use of calc() in this context.
| @media (min-width: $bp-tablet) { | ||
| margin-left: 42px; // margin-left = 32px switch width + 10px margin-left of label. | ||
| } | ||
| margin: 6px 0 0 calc(32px + 10px); |
There was a problem hiding this comment.
I'm not sure if this is what @aaemnnosttv had in mind with his suggestion - using calc() to add two unnamed values isn't really an improvement on what we had before, it's less readable and introduces an unnecessary calc() to add two static values.
Unfortunately as things stand I don't see that we can use calc() to reference the values of the relevant element dimension/properties. We could do something like this to derive the value:
diff --git a/assets/sass/components/settings/_googlesitekit-settings-group.scss b/assets/sass/components/settings/_googlesitekit-settings-group.scss
index c1625f2fb2..1eade3778c 100644
--- a/assets/sass/components/settings/_googlesitekit-settings-group.scss
+++ b/assets/sass/components/settings/_googlesitekit-settings-group.scss
@@ -55,7 +55,7 @@
color: $c-surfaces-on-surface-variant;
flex-basis: 100%;
font-size: $fs-label-sm;
- margin: 6px 0 0 calc(32px + 10px);
+ margin: 6px 0 0 $mdc-switch-track-width + $mdc-switch-label-margin-left;
}
.googlesitekit-analytics-enable-enhanced-measurement--loading {
diff --git a/assets/sass/config/_variables.scss b/assets/sass/config/_variables.scss
index e341c8792c..ef560d2231 100644
--- a/assets/sass/config/_variables.scss
+++ b/assets/sass/config/_variables.scss
@@ -308,6 +308,8 @@ $mdc-text-field-error: $c-utility-error;
$mdc-text-field-placeholder-ink-color: $c-interactive-on-disable-container;
$mdc-text-field-label: $c-interactive-on-disable-container;
+$mdc-switch-label-margin-left: 10px;
+
// Custom max width for settings notice content
$max-width-settings-notice-content: 1098px;
diff --git a/assets/sass/vendor/_mdc-switch.scss b/assets/sass/vendor/_mdc-switch.scss
index 0e0b0d0a7e..53b4a86ed7 100644
--- a/assets/sass/vendor/_mdc-switch.scss
+++ b/assets/sass/vendor/_mdc-switch.scss
@@ -40,7 +40,7 @@
font-size: $fs-body-md;
letter-spacing: $ls-s;
line-height: $lh-body-md;
- margin-left: 10px;
+ margin-left: $mdc-switch-label-margin-left;
}
.mdc-switch__track {$mdc-switch-track-width comes from the vendor's mixin and is defined here: https://github.com/material-components/material-components-web/blob/3a1767ea4da308dbee272763a377deff39cf0471/packages/mdc-switch/_variables.scss#L25
There was a problem hiding this comment.
Thanks @techanvil, I couldn't find those variables initially.
|
Size Change: +128 B (+0.01%) Total Size: 1.97 MB
ℹ️ View Unchanged
|
techanvil
deleted the
enhancement/9830-measurement-section-mobile-poc
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Addresses issue:
Relevant technical choices
@benbowler updated 9 December 2024, reviewing and fixing tests and VRTs.
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist