-
Notifications
You must be signed in to change notification settings - Fork 4
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
New Snackbar spec #306
New Snackbar spec #306
Conversation
📱 New catalog for testing generated: Download |
📱 New catalog for testing generated: Download |
📱 New catalog for testing generated: Download |
if (actionIsTooLong()) { | ||
rearrangeLayout() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rearranging the ConstraintLayout
to place the snackbar action in a new line if it exceeds its maximum length which is 128dp
@@ -0,0 +1,77 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<merge xmlns:android="http://schemas.android.com/apk/res/android" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file almost indentical to the original layout from Material library implementation
snackbar.getCustomLayout().setText(text) | ||
snackbar.setCustomAction() | ||
snackbar.showDismissActionIfNeeded(hasInfiniteDuration = snackbarLength == SnackbarLength.INDEFINITE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all these customizations are managed inside CustomSnackbarLayout
class
if (isDismissible || userShouldBeAbleToDismissSnackbar) { | ||
getCustomLayout().setOnDismissClickListener { dismiss() } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the snackbar has an indefinite duration, the user should be able to manually dismiss it. In case the action opens some screen or trigger some event that the user doesn't want to execute, then we should provide an alternative way to dismiss the snackbar by showing the dismiss button
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! I have added some questions.
@@ -52,7 +59,7 @@ open class SnackbarBuilder(view: View?, text: String) { | |||
} | |||
|
|||
@JvmOverloads | |||
open fun showCritical(snackbarLength: SnackbarLength = SnackbarLength.SHORT): Snackbar { | |||
open fun showCritical(snackbarLength: SnackbarLength? = null): Snackbar { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the default changed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the default duration is calculated when building the snackbar. See my other comment
snackbar.setCallback(callback) | ||
} | ||
areSticky() -> SnackbarLength.INDEFINITE | ||
snackbarLength == null -> if (actionListener == null) SnackbarLength.SHORT else SnackbarLength.LONG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the relation between the length and if the listener exists or not. IMHO it should default to the previous one which was SHORT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous implementation:
- There were only two possible snackbar lengths: "short" and "long".
- Even if "short" was set on purpose by the user or by default, if any action was provided, then the duration would be set to "long". This is like that by definition.
New implementation:
- One more length choice: "indefinite".
- If no duration is explicitly selected, we use the same approach as before: if no action is set, then we use "short", if an action is provided then "long" is used instead.
However, I think if (actionListener == null)
should be replaced with if (actionText == null)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it is more accurate now, thank you very much!
What is the logic to change the snackbar color ? |
There is no logic, just the way the Snackbar is shown. We have two methods: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
📱 New catalog for testing generated: Download |
📱 New catalog for testing generated: Download |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
} | ||
areSticky() -> SnackbarLength.INDEFINITE | ||
snackbarLength == null -> if (actionText == null) SnackbarLength.SHORT else SnackbarLength.LONG | ||
else -> snackbarLength |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code allows two cases that I think were not specified in the specs:
- Create a Snackbar without an action but with an indefinite length.
- Create a Snackbar with an action and five seconds length.
@yceballost Should we allow these cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spec has been updated with a more clear description, I'll update this code according to it:
- No action: 5 seconds or indefinite with a dismiss x
- Action: 10 seconds or indefinite. Optional dimiss X
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline.
The duration behaviours are explained in: Telefonica/webview-bridge#117
<ImageView | ||
android:layout_width="match_parent" | ||
android:layout_height="match_parent" | ||
android:contentDescription="@string/close_button_content_description" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yceballost @aweell According to the specs in this PR we should not include the close button which is for future development. Can you clarify it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dismiss action was out of the scope of this task initially. Telefonica/mistica-design#1476
Since the snackbar action didn't perform any custom actions apart from dismiss, the user was able to dismiss the snackbar even changing the duration to infinite.
Telefonica/mistica-design#1461 was intended and labelled as a future development.
Since we were proposing that a custom action can be performed apart from the dismiss action. We need to ensure the user can close the snackbar, that's why the addition of the close icon.
If we're okey to merge both definitions in iOS and Web (@atabel) and follow what android already implemented I'll change the specs to include both in the same task.
I apologize for the misunderstanding @idenjoe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed offline: We are going in all platform with the support of the dismiss button (the whole definition). Figma spec has been updated so I'm going to adjust this code a little bit. I'll re-request a new review soon
📱 New catalog for testing generated: Download |
📱 New catalog for testing generated: Download |
} | ||
} | ||
if (alwaysShowDismiss.isChecked()) { | ||
withDismiss() | ||
} | ||
val duration = when { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can replace "5 seconds" & "10 seconds" radio button by "Default" in catalog 🤔
- Default
- Indefinite lenght
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it makes sense. Then I would replace the whole radio group with just a checkbox: ☑️ Indefinite length. WDYT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect!
📱 New catalog for testing generated: Download |
🥅 What's the goal?
New Snackbar definition. Main changes:
🚧 How do we do it?
The new dismiss button is included in the Material 3 definition but it is not supported yet in the Material Android library that we are using. Instead I had to overwrite the layout of Snackbar class and do some adaptations to include this new button and logic:
Snackbar::snackbarLayout
that is fortunately a public method and remove all of its child views☑️ Checks
🧪 How can I test this?