Skip to content

Update ConnectButton visuals#136

Merged
lzanita09 merged 4 commits intomasterfrom
connect_button_ui_updates
Jan 22, 2020
Merged

Update ConnectButton visuals#136
lzanita09 merged 4 commits intomasterfrom
connect_button_ui_updates

Conversation

@lzanita09
Copy link
Copy Markdown
Collaborator

  • Copy changes
  • Button size tweaks
  • Simplify TextView padding adjustment logic
  • Always draw ConnectButton border

* Copy changes
* Button size tweaks
* Simplify TextView padding adjustment logic
* Always draw ConnectButton border
@lzanita09 lzanita09 requested a review from priyaashah January 16, 2020 23:22
this.onDarkBackground = onDarkBackground;
this.startIconBackgroundColor =
onDarkBackground ? ContextCompat.getColor(context, R.color.ifttt_start_icon_background_on_dark)
: Color.BLACK;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this intentional? This color is used below, if we don't set it in the constructor, how can we use it where required?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We no longer use this field because we are going to use the backgroundColor, which will be set later on in the process, to derive the background color for the start icon.

@priyaashah
Copy link
Copy Markdown
Contributor

I am assuming all the dimens are same as those for the connect button in the app (https://github.com/IFTTT/IFTTTAndroid/pull/3924)

@lzanita09
Copy link
Copy Markdown
Collaborator Author

Yep.

<?xml version="1.0" encoding="utf-8"?>
<resources>
<dimen name="ifttt_text_padding_horizontal">72dp</dimen>
<dimen name="ifttt_text_padding_horizontal">@dimen/ifttt_connect_button_height</dimen>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would say we could leave this as 72dp instead of setting it as the connect button height. Those two dimens are unrelated and if we decide to change height, this will change too

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

They are related actually, the padding should match exactly how tall the connect button is.

@lzanita09 lzanita09 merged commit 85f9ed2 into master Jan 22, 2020
@lzanita09 lzanita09 deleted the connect_button_ui_updates branch January 22, 2020 02:18
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.

2 participants