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.

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

[ABW-3539] UI Adjustments - Account permission #1221

Merged
merged 2 commits into from
Jul 30, 2024

Conversation

danvleju-rdx
Copy link
Contributor

@danvleju-rdx danvleju-rdx commented Jul 17, 2024

Jira ticket: ABW-3539

Description

Updates padding for Account Permission screen.
Account cards updates for AccountPermissionChooseAccounts are handled here.

Screenshot

Screen 1 Before Screen 1 After
Simulator Screenshot - iPhone 15 Pro - 2024-07-17 at 17 45 33 Simulator Screenshot - iPhone 15 Pro - 2024-07-17 at 17 39 31

Copy link
Contributor

@matiasbzurovski matiasbzurovski left a comment

Choose a reason for hiding this comment

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

Looks to me we need Zeplin updates before implementing this.

@@ -44,7 +44,7 @@ extension AccountPermission {
) { viewStore in
GeometryReader { geometry in
ScrollView {
VStack(spacing: .medium2) {
VStack(spacing: .large1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this value is correct since Zeplin is indicating a padding to a wrong part of the view
Screenshot 2024-07-17 at 16 58 17

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured out the actual distance using an unconventional method :D. The selected text box in the screenshot above has a height of 69pt and 3 rows, so 1 row is 23pt. I subtracted 23 from 63 and got 40. Pretty sure this is the correct distance :)

Copy link
Contributor

Choose a reason for hiding this comment

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

but are we sure that 69 represents 3 lines? and not just a random box that happens to look like it represents 3 lines from the Text we need to calculate the distance?

I believe we should push for having updated and consistent designs before making these changes. Not only it would be easier for us, but also state the source of truth if implementation differs among platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, asked Aftab

Text(L10n.DAppRequest.AccountPermission.updateInSettingsExplanation)
.foregroundColor(.app.gray2)
.textStyle(.body1Regular)
.multilineTextAlignment(.center)
.padding(.horizontal, .medium2)
.padding(.top, .large1)
Copy link
Contributor

@matiasbzurovski matiasbzurovski Jul 17, 2024

Choose a reason for hiding this comment

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

Zeplin indicates the distance between the two elements should be 76, would be .large1 + 36 (which we don't have any unit * 9). Still, this distance seems quite arbitrary, so I would confirm with Aftab that is a fixed distance and not that we want it centered (as it currently shows).

Screenshot 2024-07-17 at 17 12 43

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.large1 from VStack spacing + this .large1 will be 80. I recall Aftab agreed that we should round to the nearest multiple of 4.
Regarding fixed distance vs centered, I will confirm with Aftab and the Android team.

Copy link
Contributor

Choose a reason for hiding this comment

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

but 76 is also multiple of 4, that could be the expected distance as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed with Aftab and he adjusted the spacing to .medium1

@GhenadieVP GhenadieVP added the DO NOT MERGE Merging is blocked or prohibited due to missing specs or other higher priority PRs label Jul 22, 2024
@GhenadieVP GhenadieVP removed the DO NOT MERGE Merging is blocked or prohibited due to missing specs or other higher priority PRs label Jul 29, 2024
@GhenadieVP GhenadieVP merged commit 74c6cf2 into main Jul 30, 2024
6 checks passed
@GhenadieVP GhenadieVP deleted the ABW-3539-ui-adjustments-account-permission branch July 30, 2024 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants