[PM-16573] Tweak Two-Factor Notice design #1234
Conversation
|
Tests are currently failing pending #1235 |
| // Despite the image being 96x96, the actual dimensions of the | ||
| // illustration within that are 96x72 (4:3). | ||
| // So to make how it interacts with other elements correct, | ||
| // we have to apply negative padding equal to 1/8 of the width | ||
| // to both top and bottom, thus making the image the "correct" size | ||
| Asset.Images.Illustrations.userLock.swiftUIImage | ||
| .resizable() | ||
| .frame(width: 124, height: 124) | ||
| .padding(.vertical, -15.5) |
There was a problem hiding this comment.
❓ I could be missing something, but isn't the 24 points of spacing in the designs from the bottom of the image (including the padding in the image), not from the bottom of the person in the image? Or is the negative padding serving a different purpose?
There was a problem hiding this comment.
How are you getting those numbers indicating the distance? I've been messing with Figma forever to get something like those, to no avail
I was going off of this comment in the design review, which to me implied that the padding was meant to be between the bottom of the illustration itself, not the full square of it:
There was a problem hiding this comment.
Figma makes this more difficult than it should be 😒. If you select an element, hold down option (⌥), and move your cursor to another element, it will show the red spacing amounts.
There was a problem hiding this comment.
Aha! Yeah, I totally missed that :(
I can adjust it to be 24 from the square-image, though I think I still want to keep the handling of rotation bespoke rather than futzing with making the PageHeaderView dynamic to accommodate all the differences it has from this design. (Though a question to bring back to Design might be, do we want to unify these values across the app, as we have these screens, and then the intro carousel, and then the places where PageHeaderView are being used, which all are doing essentially the same thing, just with different values and sizes, and should they actually all be the same?)
There was a problem hiding this comment.
🤔 IMO we should always try to use PageHeaderView for these kind of layouts so we reuse as much code as we can that then it's much easier to maintain and change in several places at the same time if another reskin/revamp happens in the future.
Ideally, I'd think the view to be the same everywhere we use it in the app so I'd definitely bring this up to design to see if we can apply the change to all the places it's currently used.
However, if design wants to have different "flavors" of this header to accommodate better the controls depending on the actual view/context then we could have different "modes" or "layout types" the PageHeaderView can adjust to; although I would prefer to have only one.
🤔 Regarding the image and its padding, I would prefer padding to depend on the control and not the image being used whenever possible. If the image file has some odd padding internally I'd ask design if they can provide the same image with the padding adjusted so we can apply the same margin Figma shows without having to consider anything regarding the image itself for it.
There was a problem hiding this comment.
Hah, for whatever reason I didn't even think of making it PageHeaderView "modes", and was wanting to make all the different things configurable 🙃 I can even see if I can make it work like our general style guide handling, now
I agree, moving it then to having different modes there is the way to go—and then in the future that will make it easier to evaluate and decide if we actually want to shift to just having one of them, in one place.
Regarding the image and its padding
Good point. I can adjust.
|
No New Or Fixed Issues Found |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1234 +/- ##
==========================================
- Coverage 89.66% 89.66% -0.01%
==========================================
Files 718 719 +1
Lines 45428 45429 +1
==========================================
Hits 40732 40732
- Misses 4696 4697 +1 ☔ View full report in Codecov by Sentry. |
| /// An `OrientationBasedValue` encapsulates values that might be different | ||
| /// for rendering based on orientation, such as image size or space between text. | ||
| struct OrientationBasedValue<T: Equatable & Sendable>: Equatable, Sendable { |
|
@fedemkr There was a conflict with the changes you had made to the |
| .if(style.imageColor != nil) { view in | ||
| return view.foregroundStyle(style.imageColor!) | ||
| } |
There was a problem hiding this comment.
🤔 Could force unwrapping be avoided here somehow?
There was a problem hiding this comment.
I tried doing a guard let for it, but ran into issues with trying to return view out of the else block. There might be some other way to do this better, though; this was just the one I was able to find.
|
|
||
| /// An `OrientationBasedValue` encapsulates values that might be different | ||
| /// for rendering based on orientation, such as image size or space between text. | ||
| struct OrientationBasedValue<T: Equatable & Sendable>: Equatable, Sendable { |
There was a problem hiding this comment.
🤔 Love this! And could this OrientationBasedValue struct be extracted to its own file in Platform folder? I see its potential to be reused on other components/views as well.
🤔 Could we have a convenience init to fill portrait and landscape with the same value just by passing one argument? So we don't have to repeat them and is easier to set up, like in largeTextTintedIcon.
There was a problem hiding this comment.
🤦🏼♀️ I meant at some point to potentially pull some of these out into their own files, yeah. That one seems an obvious candidate for it.
I like the idea for a convenience init! I can get on that!
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-16573
📔 Objective
This addresses a number of issues raised during design review on the Two-Factor notice added for PM-8216. In the process, I stopped using the
PageHeaderViewbecause there were enough differences in sizes—particularly on the "set up two-factor" screen—that it would have made that view take a lot of parameters, and it ultimately made more sense to me to just build it bespoke for each screen to accommodate the specifics of each.📸 Screenshots
⏰ 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