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

ANDROID-14803 Update all the 24dp lateral margins to 16dp #372

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

pmartinbTEF
Copy link
Contributor

@pmartinbTEF pmartinbTEF commented Jul 11, 2024

🥅 What's the goal?

Check that all sides margins of Mistica elements are 16dp (and not 24dp)

🚧 How do we do it?

-Checked Mistica catalog using layout inspector to check sides margins
-Replaced 24dp with 16dp

Only found empty state xml implementation which used 24dp of side margins
I also found that link button of card_actions_view uses a negative margin. I think this should be reviewed in another task

☑️ Checks

  • I updated the documentation, including readmes and wikis. If this is a breaking change, tag the PR with "Breaking Change" label and remember to include breaking change migration guide in release notes where this version is released.
  • Tested with dark mode.
  • Tested with API 24.
  • Sync done with iOS team for this feature to ensure alignment, if applies.

🧪 How can I test this?

  • Open Mistica catalog using layout inspector
  • Check that sides margins are 16dp

Before
drawing

After
drawing

Copy link

📱 New catalog for testing generated: Download

@@ -69,15 +66,15 @@
android:id="@+id/empty_state_screen_primary_button"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginStart="24dp"
android:layout_marginEnd="24dp"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing until you find it's meaning, perhaps it could be extracted to a dimension margin_between_buttons or someting like that.

Another perhaps better approach would be to leave them inside the ConstraintLayout and create a chain and set the paddings between elements.

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 prefer the first option because it doesn't imply changing the code. Done!

android:paddingEnd="24dp"
android:paddingBottom="24dp"
android:paddingEnd="16dp"
android:paddingBottom="16dp"
Copy link
Contributor

@dpastor dpastor Jul 11, 2024

Choose a reason for hiding this comment

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

padding bottom should be modified? aren't just lateral margins?

Copy link

Choose a reason for hiding this comment

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

We should go with 16dp to match when the button is in the bottom on the screen the spacing the button fixed has.

Copy link

@aweell aweell left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dpastor dpastor left a comment

Choose a reason for hiding this comment

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

👍

@pmartinbTEF pmartinbTEF requested a review from jeprubio July 15, 2024 08:34
Copy link

📱 New catalog for testing generated: Download

@pmartinbTEF pmartinbTEF merged commit 9ba226c into main Jul 15, 2024
5 checks passed
@pmartinbTEF pmartinbTEF deleted the ANDROID-14803-Sides_margins_16 branch July 15, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants