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

Jump to unread: implement new design #3547

Merged
merged 6 commits into from Jun 28, 2021

Conversation

ganfra
Copy link
Contributor

@ganfra ganfra commented Jun 22, 2021

Implements new design for Jump to unread.
Also quick fix some issues with his visibility not being refreshed correctly (its more a temporary solution waiting for reworking all that...)

@ganfra ganfra marked this pull request as draft June 22, 2021 14:30
@ganfra ganfra requested a review from bmarty June 24, 2021 09:25
@ganfra ganfra marked this pull request as ready for review June 24, 2021 09:25
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Some small remarks, else LGTM.
Also would be nice if you add a quick screenshot with the change, for posterity :)


<attr name="jumpToUnreadStyle" format="reference" />

<style name="Widget.Vector.JumpToUnread.Base" parent="Widget.MaterialComponents.Chip.Action">
Copy link
Member

Choose a reason for hiding this comment

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

I would have called this style: Base.Widget.Vector.Chip.Action.JumpToUnread, but anyway it's OK like that :)

@@ -86,6 +86,7 @@
<item name="snackbarButtonStyle">@style/Widget.Vector.SnackBar.Button</item>
<item name="snackbarTextViewStyle">@style/Widget.Vector.SnackBar.TextView</item>
<item name="actionModeStyle">@style/Widget.Vector.ActionMode</item>
<item name="jumpToUnreadStyle">@style/Widget.Vector.JumpToUnread.Dark</item>
Copy link
Member

Choose a reason for hiding this comment

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

This section is for the default style of the system Widget.

Maybe move this new style at the end of the theme declaration (after vctr_social_login_button_gitlab_style)

@@ -86,6 +86,7 @@
<item name="snackbarButtonStyle">@style/Widget.Vector.SnackBar.Button</item>
<item name="snackbarTextViewStyle">@style/Widget.Vector.SnackBar.TextView</item>
<item name="actionModeStyle">@style/Widget.Vector.ActionMode</item>
<item name="jumpToUnreadStyle">@style/Widget.Vector.JumpToUnread.Light</item>
Copy link
Member

Choose a reason for hiding this comment

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

Same remark

<?xml version="1.0" encoding="utf-8"?>
<resources>

<attr name="jumpToUnreadStyle" format="reference" />
Copy link
Member

Choose a reason for hiding this comment

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

Can you rename to vctr_jump_to_unread_style to follow the current naming convention of custom attributes of this project?

android:layout_width="0dp"
<com.google.android.material.chip.Chip
android:id="@+id/jumpToReadMarkerView"
style="?attr/jumpToUnreadStyle"
Copy link
Member

Choose a reason for hiding this comment

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

attr/ is optional here

android:paddingBottom="12dp"
android:text="@string/room_jump_to_first_unread"
android:textColor="?colorOnPrimary"
app:drawableStartCompat="@drawable/arrow_up_circle" />
Copy link
Member

Choose a reason for hiding this comment

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

This resource (5 png files!) can be removed too.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the update, LGTM!

@bmarty bmarty merged commit edd24de into develop Jun 28, 2021
@bmarty bmarty deleted the feature/fga/jump_to_unread_redesign branch June 28, 2021 14:45
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.

None yet

2 participants