-
Notifications
You must be signed in to change notification settings - Fork 985
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
Button Refactors Final Final #16772
Button Refactors Final Final #16772
Conversation
@@ -3,101 +3,87 @@ | |||
|
|||
(defn custom-color-type | |||
[customization-color icon-only?] | |||
{:icon-color colors/white | |||
:icon-secondary-color colors/white-opa-70 |
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.
icon-secondary-color
is not needed as the icon is the label color for icon only variant 👍
@@ -8,39 +8,35 @@ | |||
:bottom 0}) | |||
|
|||
(defn icon-style | |||
[{:keys [icon-container-size icon-background-color icon-container-rounded? |
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.
icon background color is not actually a thing afaik so I removed 👍
Jenkins BuildsClick to see older builds (18)
|
78aed34
to
2c11d37
Compare
:padding-horizontal (when-not (or icon-only? icon-left icon-right) | ||
(case size | ||
56 16 | ||
56 (if border-color 10 11) |
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.
fixes padding for icon above variant
40 9 | ||
32 5 | ||
3)) | ||
40 (if border-color 8 9) |
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.
fixes alignment for outline variant
:type type | ||
:size 32 | ||
:accessibility-label accessibility-label | ||
:background icon-background} |
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.
there is a better solution for this, however this component needs to be revisited as it can be much simpler if we encapsulate the colors/designs correctly in the component 👍
@@ -1,7 +1,5 @@ | |||
(ns quo2.components.record-audio.record-audio.buttons.record-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.
nit: if anyone feels like doing some refactoring, these components are in the wrong part of the codebase :) should be aligned with figma file structure convention 👍
@@ -63,14 +63,14 @@ | |||
[extra-action-view extra-action extra-text extra-action-selected?] | |||
[rn/view {:style style/buttons-container} | |||
[quo/button | |||
{:type :grey | |||
:style {:flex 0.48} ;;WUT? 0.48 , whats that ? |
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 WUT component is also some other work that should be address. This is acutally another component in the design system that should be added and used instead ->
#15006 still relevant?
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 work @J-Son89. The code looks good.
I experimented with the button component in the preview area and found some rendering issues, but I haven't checked if they existed before. I'm approving the PR because I think you'll either fix them in this PR or create issues to be fixed in the near future :)
- Choosing a customization color doesn't change the button color. Maybe a problem just with the preview code, not the component itself.
- When the
show-icon-top
option is true, the button looks like Fig 1. It gets worse the smaller the button's size. - When the
show-icon-top
andshow-icon-right
options are true, the button looks like Fig 2. If this combination of states are not possible, I think the code should reflect this. So ideally, only the possible variations in Figma should be rendered, and non-supported variations should just default to a good default state. Better have something correctly rendered per design, than something clearly broken. This I think should be part of our recommendations for writing components.
:padding-left (when-not (or icon-only? icon-left) | ||
(case size | ||
56 16 | ||
56 nil |
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.
A bit weird to see a condition that returns nil, but I understand you probably kept 56
here to be perfectly symmetrical with the other case
s that do have a non-nil value for 56
.
Still, it's a bit rare to see in Clojure a case
condition that explicitly returns nil because you can just add a default condition. Having the default condition is also more robust in the long run because the default value will prevent the code from throwing an exception when no size matches.
I know I keep repeating this, but case
calls should have a default value of nil
most of the time (I mean in particular for status-mobile, a CLJS-RN app). Imagine a designer adds a new size to Figma and that we devs update the majority of the code to handle this new size, but we fail to update certain case
calls that don't have default values (such as the case
s in this namespace) and boom, the code throws an exception.
Even if we gracefully handle the exception (which we don't now), I believe most users prefer to not see failures in apps, and are more receptive to a styling issue if it's not grotesque of course.
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.
yep, well padding-left will overwrite it otherwise - sure I can update the default case to nil 👍 but there is default values here? 🤔
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 point @J-Son89. If there's no default value, the ideal scenario to me is to log and return nil. Unmatched conditions that will look wrong shouldn't go unnoticed. Of course, one may argue that nobody will see the logs, but it surely helps devs to more proactively catch these issues instead of real users, which would be bad.
Trusting that the code will pass only valid sizes now and in the future is a recipe for user bugs with case
, so I would avoid that.
This idea to return nil to avoid the exception with case
and log something if the unmatched case is unexpected is a good practice in general, so I think it doesn't hurt to do it. To be fair, I haven't done this in status-mobile, but I'll keep this practice in mind.
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.
Oh well this is quite specific to setting up this component because here padding horizontal can clash with padding left and padding right. Ideally this code could be written to just avoid the use of padding horizontal here but that would involve me redesigning the button styling and so I don't want to invest that effort as there is a lot variations to check and I have invested a lot of time in this component already. But there is definitely a cleaner approach here
:disabled (not= state :valid)}) | ||
(def button-view-profile | ||
{:margin-top 24 | ||
:width "100%" |
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.
Suspicious 100%
?
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 is fine, I checked with Figma and it looks correct. Flex stretches the wrong way and I would have to adjust this component more than is necessary for this pr otherwise so it's the safest approach for what was already a misuse - it had a fixed width which was wrong 👍
@ilmotta - not entirely sure what we should do here. These are both misconfigurations of the component. When icon-above is set there is no need for icon-right or icon-left (also the size can only be 56) - I can update the component a bit to handle this 👍 Alternatively we can handle this in the preview screens? which is better. I am not sure we should build in this complexity to our quo2 components, I think as long as we're matching a configuration from figma then it should be acceptable 🤔 |
Perhaps the preview screens could provide some validation for misconfigurations or even better if we could have some sort of types validation to our components, this would be a better approach imo. It seems like adding too much guard rails into the code of our design systems components adds unnecessary complexity for little benefit of what should be well designed code and aware developers that understand how we can best leverage using a design system approach. |
2c11d37
to
b81ad3e
Compare
You mean the 3rd point? I think @J-Son89, you are correct and I agree the complexity of the component code can grow, although marginally in my (single) experience with the |
Yeah I think this is best to have as a discussion first :) |
Hey @J-Son89 thanks! |
will check, thanks @ulisesmac ! |
85e3f2a
to
945afe9
Compare
Outline color of the button itself looks right but I can confirm the color of the icons seems darker than it should be, the correct color is Neutral/Solid/50
|
ISSUE 1 Enable notifications and Start using status buttons are missing background when user has signed in by syncing
ISSUE 2 Wrong buttons' colours in Activity Center
ISSUE 3 Unread counter brakes layout of AC icon
ISSUE 4 Broken (stretched) cancel reply button
ISSUE 5 Wrong color of composer buttons As such I think it is best to leave for now and I can do in a follow up. If you would like more details on this let's discuss off of Github as it will be easier to explain in details :) ISSUE 6 Wrong buttons' background on community home screen (Android)
|
@J-Son89 thanx for the fixes! Will check and provide feedback. Regarding
I have checked community that has background image set and button's background still does not look correct to me. For comparison here is how it looks on IOS Here how it looks in Figma Seems like both IOS and Android buttons' background colour differ from design in this particular case. I guess we need another input from @Francesca-G on this issue. |
Yeah, there is some blur on this so it really depends on the background to match with Figma. If @Francesca-G can check that would be great :) |
80% of end-end tests have passed
Failed tests (8)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (32)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
@pavloburykh @J-Son89 I can confirm both buttons in the top page nav have a blur, so the color will slightly change based on the cover image. Light mode: button color is White/40% with background blur effect Blur 40 When a context menu or a drawer is open there's an overlay layer applied to the background page but that shouldn't affect the blur or the color of the buttons. |
Thank you for your review @Francesca-G @J-Son89 based on that, the behaviour on Android device looks incorrect, because overlay layer affects the blur and color of the buttons. telegram-cloud-document-2-5352946714577743856.mp4 |
80% of end-end tests have passed
Failed tests (1)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (4)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
|
Awesome, thanks @pavloburykh. I'm looking into it today and will keep you updated! |
@pavloburykh, @Francesca-G regarding to issue 6. Would it be okay to leave this as a follow up work? It is specific to the communities page and is only there as the buttons are correctly set now. There is some work that needs to be done on that scrolling page and will be cleaner if it is done separately. Or it could be done as part of the work related to 1.24 design review - https://www.figma.com/file/Tf5nfkYvpbnNCo4rKLK7lS/Feedback-for-Mobile?type=design&node-id=3674-338290&mode=design&t=1ZmsbGWnOi95yo11-4 |
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.
@J-Son89 follow up required :) thank you!
@J-Son89 thank you! I will create separate tickets for ISSUE 5 and 6. PR is ready for merge. |
Thanks @pavloburykh, you can assign issue 5 to me and I can start it this week 👍 |
396b934
to
d8d5c41
Compare
fixes: #16535
This pr updates the use of the button component.
I renamed a lot of props to align better with Figma and best practices
i.e
prop names
icon
->icon-only?
before
->icon-left
after
->icon-right
above
->icon-top
style
->container-style
disabled
->disabled?
removed all uses of combined types, e.g
blur-bg
->type :grey
&:background :blur
also I removed some internal styles as they were duplicates an not needed.
Updated all uses across the codebase to now use this api
Also fixed an alignment issue with the outline button variant where it was not sitting correctly
To Test: Each and every button
Additionally I found a number of uses of customization color for in the application that were not set correctly, this meant on first login the user's color was not showing. Now it is using the right
sub
for data and should show 👍