-
Notifications
You must be signed in to change notification settings - Fork 309
Introduce StreamButton and StreamIconButton #6073
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
base: v7
Are you sure you want to change the base?
Conversation
SDK Size Comparison 📏
|
66df1ab to
8724d33
Compare
| * limitations under the License. | ||
| */ | ||
|
|
||
| package io.getstream.chat.android.compose.ui.components.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.
I followed the existing package structure, but if we decide to extract this as part of the core design system, we should change the package to a more generic one
| internal fun StreamButtonStyle.border(enabled: Boolean) = | ||
| if (enabled) border else disabledBorder | ||
|
|
||
| internal object StreamButtonStyleDefaults { |
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.
For how the tokens are structured at the moment, these styles are not trivial to extract away from this repository. The reason is that, as far as I understand, the colors in Figma all belong to the same set which contains both "design system" generic colors & chat-specific ones.
So with the current state, what we could extract is everything except the actual instantiation of the button styles, i.e. this object.
| @Suppress("ObjectPropertyNaming") | ||
| internal object StreamSpacings { | ||
| val none = 0.dp | ||
| val _2xs = 4.dp |
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.
maybe we should follow the naming here as
xxs instead of _2xs
xxl or xxxl instead of _2xl or _3xl
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 thought about that, but my main concern is that it makes it easier to mistake them 🤷♂️. But in the end, when these are autogenerated, we'll inherit what comes from Figma, so maybe we can check with Jurgen
| onClick: () -> Unit, | ||
| modifier: Modifier = Modifier, | ||
| enabled: Boolean = true, | ||
| style: StreamButtonStyle = StreamButtonStyleDefaults.primarySolid, |
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.
In Figma Primary is the type and solid is the style. I think it's better if we keep them as separate variables here as well. I have a comment in figma that not all types implement all styles.
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.
At the moment, StreamButtonStyle contains a complete styling description for the button and for rendering we just take the values as they are.
If we split it into, let's say, ButtonType and ButtonStyle, what would you put in each? I guess we could decide colors based on the type and then conditionally apply them to the container/border/content based on the style, but wouldn't that add some complexity at rendering time that we can avoid? 🤔
| val primarySolid: StreamButtonStyle | ||
| @Composable | ||
| get() { | ||
| val colors = ChatTheme.colors |
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 we want to extract colors from ChatTheme into a more general StreamTheme so the colors can also be used for other products later.
What I'm thinking is to create a primary app color separate from the primary button color, so you probably only have to set your app color and not all the colors on all the components per se.
So it becomes: colors.buttonTypePrimaryBg ?? colors.primaryColor
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 we want to extract colors from ChatTheme into a more general StreamTheme so the colors can also be used for other products later.
I agree, but I have a concern (basically what I mention here). Atm, in Figma there's no distinction between design system and chat-specific tokens (right?), so all the colors live in the same place.
What I'm thinking is to create a primary app color separate from the primary button color, so you probably only have to set your app color and not all the colors on all the components per se.
I'm a bit worried if we deviate from Figma that it will make it more likely to have inconsistencies. I think that if we want this, we should do it in Figma, as we can easily have tokens referencing other tokens, and then trickle it down to the code.
So it becomes: colors.buttonTypePrimaryBg ?? colors.primaryColor
If we go for this, I'd centralize this check in the colors instantiation rather than in component-specific code. Meaning the button (and any other component) doesn't need to know each default and doesn't perform any check at rendering time. For example a primary button would always uses buttonTypePrimaryBg, which has its default already resolved and only once, when Chat colors were instantiated.
| import io.getstream.chat.android.compose.ui.theme.StreamSpacings | ||
|
|
||
| @Composable | ||
| internal fun StreamIconButton( |
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.
Is IconButton a different component, or is IconButton just a regular StreamButton with no label?
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 made them different components to enforce padding and shapes, but I can check what it would take to merge them into one
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.
ca183f4 to
5bf6948
Compare
5bf6948 to
2936056
Compare
|




🎯 Goal
AND-995: Implement new buttons
Introduce
StreamButtonandStreamIconButton🛠 Implementation details
I created base buttons kinda following the familiar Material/Compose API but with Stream-specific styling. They are not meant to be arbitrarily flexible. Rather, they are made to follow closely the Stream design system. And anyway, as we discussed for similar cases, integrators will be able to either customize colors through the theme or replace the component altogether.
Nothing is public for the time being, until we decide if we actually want to expose these base buttons to integrators.
🎨 UI Changes
🧪 Testing
You can go to the previews and launch them on a device.
☑️Contributor Checklist
General
Code & documentation
☑️Reviewer Checklist
🎉 GIF
Please provide a suitable gif that describes your work on this pull request