-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat: Action Widgets Reskinning - Button, Button Group, Icon Button, Menu Button #17093
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Unable to find test scripts. Please add necessary tests to the PR. |
/ok-to-test sha=2f2cfee |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3135784459. |
@@ -17,7 +17,7 @@ export const CONFIG = { | |||
text: "Submit", | |||
buttonVariant: ButtonVariantTypes.PRIMARY, | |||
placement: ButtonPlacementTypes.CENTER, | |||
rows: 4, | |||
rows: 5, |
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.
@dilippitchika @dhruvikn Are we sure we want to increase button height? It looks odd to me as compared to standard button sizes.
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 is also worth noting that the current height of buttons with row = 4 is 32px, and not 30px, as mentioned in the PRD.
Therefore, when changing row to 5, we will get a button with a height of 42px, not 40px.
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.
Had a discussion with @dilippitchika, @somangshu, and @jsartisan. We're gonna be sticking with row = 4 and button height = 32px. Will be reverting the change.
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3142905368. |
/ok-to-test sha=8eafc8a |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3143410546. |
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.
LGTM
Deployment failed with the following error:
|
/ok-to-test sha=b301eaa |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3149415525. |
/ok-to-test sha=4ea6a76 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3154112978. |
/ok-to-test sha=a573e07 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3156545334. |
/ok-to-test sha=0f3167e |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3156761433. |
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3156761433. Click to view performance test results
|
Description
This PR fixes UI inconsistencies in the following Action Widgets -
Fixes #10972
Fixes #15170
Fixes #10989
Fixes #10996
Type of change
How Has This Been Tested?
Checklist: