fix: disable cancel button on a disabled select widget #13275
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
github-actions
bot
added
Widgets & Blocks Pod
| fillColor={disabled ? Colors.GREY_7 : Colors.GREY_10} | ||
| name="cross" | ||
| onClick={handleCancelClick} | ||
| onClick={!disabled ? handleCancelClick : undefined} |
There was a problem hiding this comment.
This logic should be present in the icon and not here. If I were to use disabled in Icon at other places then I would have to explicitly write this logic everytime and there is a good chance that someone might just miss out doing this expecting it might be happenning internally or just not being mindful.
| fireEvent.click(getByTestId("selectbutton.btn.cancel")); | ||
| expect(defaultProps.handleCancelClick).not.toBeCalled(); | ||
| expect(getByText("0")).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
Add a new line after this.
|
/ok-to-test sha=2d57840 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2232770080. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2232770080. Click to view performance test results
|
1 similar comment
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2232770080. Click to view performance test results
|
| @@ -197,7 +197,11 @@ export const IconWrapper = styled.span<IconProps>` | |||
|
|
|||
| display: flex; | |||
| align-items: center; | |||
|
|
|||
| cursor: ${(props) => (props.disabled ? "not-allowed" : "pointer")}; | |||
| &:hover { | |||
There was a problem hiding this comment.
Can we do &, &:hover? It would cover both.
Also, do we need to define hover styles? IIRC cursors values get inherited; so we probably can remove the hover styles, can you check this out?
| <IconWrapper | ||
| className={Classes.ICON} | ||
| clickable={clickable} | ||
| onClick={undefined} |
There was a problem hiding this comment.
Do we need undefined here? We took out onClick from props
|
/ok-to-test sha=47398e2 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2238975317. |
|
/ok-to-test sha=70c475f |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2243495415. Click to view performance test results
|
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2243495415. Click to view performance test results
|
|
Tested this PR and working as expected
|
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2243495415. |
|
/ok-to-test sha=de8aebf |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2291611828. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2243495415. Click to view performance test results
|
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2291611828. Click to view performance test results
|
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2243495415. Click to view performance test results
|
|
/ok-to-test sha=fdc3143 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2292960420. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2292960420. Click to view performance test results
|
|
/ok-to-test sha=2f8c178 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2304320947. |
| @@ -248,7 +248,6 @@ function ManualUpgrades() { | |||
| > | |||
| <Icon | |||
| className="t--upgrade" | |||
| disabled={applicationVersion < latestVersion} | |||
There was a problem hiding this comment.
@arunvjn Could you help confirm this.
I had two failing test, Application_URL and Git_Spec. which were caused by disabled been true and still expecting the onClick Event to fire. So I basically removed the disabled property since it needs onClick to fire regardless
There was a problem hiding this comment.
This looks good. @Tooluloope
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2304320947. Click to view performance test results
|
1 similar comment
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2304320947. Click to view performance test results
|
Tooluloope
deleted the
fix/disable-cancel-button-on-a-disabled-select-widget
branch
Description
Disable the cancel button on a disabled select widget
Fixes #13254
Type of change
How Has This Been Tested?
Checklist:
Test coverage results 🧪
🟢 Total coverage has increased