PM-18861: Added title accessory content view for BitwardenMenuField #1406
Conversation
There was a problem hiding this comment.
I couldn't make snapshot testing to wait/delay for BitwardenMenuField to calculate the title width.
There was a problem hiding this comment.
I think you can make this work if you record the snapshot in the key window.
When we build the config:
ios/GlobalTestHelpers/Extensions/Snapshotting.swift
Lines 22 to 29 in 920acfd
You can add drawHierarchyInKeyWindow: true as a parameter to .image(...):
.image(
drawHierarchyInKeyWindow: true,
precision: defaultPrecision,
perceptualPrecision: defaultPerceptualPrecision,
layout: .device(config: .iPhone13(.portrait)),
traits: UITraitCollection(userInterfaceStyle: .light)
)
We probably don't want to do this for all snapshots though. So maybe we could add it as a parameter to the portrait()/portraitDark()/tallPortraitAX5() functions?
There was a problem hiding this comment.
this did work! thanks!
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1406 +/- ##
==========================================
- Coverage 89.67% 85.81% -3.87%
==========================================
Files 766 983 +217
Lines 48109 58446 +10337
==========================================
+ Hits 43141 50154 +7013
- Misses 4968 8292 +3324 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| .overlay { | ||
| if let titleAccessoryContent { | ||
| titleAccessoryContent | ||
| .frame( | ||
| maxWidth: .infinity, | ||
| maxHeight: .infinity, | ||
| alignment: .topLeading | ||
| ) | ||
| .offset(x: titleWidth + 4, y: 12) | ||
| } | ||
| } |
There was a problem hiding this comment.
❓ Why does this need to be in overlay? Can't it be just a HStack with the title Text and the title Accessory Content?
There was a problem hiding this comment.
@fedemkr in this case, the titleAccessory contains a button, and placing a button inside a Menu's label: {} does not trigger the click action.
| if let titleAccessoryContent { | ||
| titleAccessoryContent | ||
| .frame( | ||
| maxWidth: .infinity, | ||
| maxHeight: .infinity, | ||
| alignment: .topLeading | ||
| ) | ||
| .offset(x: titleWidth + 4, y: 12) |
There was a problem hiding this comment.
🎨 Could we align this so it's vertically center aligned with the title? As the font sizes get larger it looks a little out of place being top-aligned.
There was a problem hiding this comment.
this is overlay and it is not in the same HStack with the title, one thing I could try is get the height of the title as we get the width, and try to manually make it aligned with title.
There was a problem hiding this comment.
@matt-livefront now after doing couple of testing, I don't believe that manually aligning is a good approach. We can't determine the actual height of the titleAccessoryContent, and I prefer not to hard code the height of the icon.
There was a problem hiding this comment.
I think maybe you could change the button style for a similar one to have the size based on the font size, so the icon gets bigger on large accessibility fonts thus appearing more aligned with the text.
There was a problem hiding this comment.
I think you can make this work if you record the snapshot in the key window.
When we build the config:
ios/GlobalTestHelpers/Extensions/Snapshotting.swift
Lines 22 to 29 in 920acfd
You can add drawHierarchyInKeyWindow: true as a parameter to .image(...):
.image(
drawHierarchyInKeyWindow: true,
precision: defaultPrecision,
perceptualPrecision: defaultPerceptualPrecision,
layout: .device(config: .iPhone13(.portrait)),
traits: UITraitCollection(userInterfaceStyle: .light)
)
We probably don't want to do this for all snapshots though. So maybe we could add it as a parameter to the portrait()/portraitDark()/tallPortraitAX5() functions?
🎟️ Tracking
PM-18861
📔 Objective
BitwardenMenuFieldto add accessory content right next to title.Username type📸 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