Skip to content
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

fix: table widget design #8426

Merged
merged 21 commits into from
Oct 26, 2021
Merged

fix: table widget design #8426

merged 21 commits into from
Oct 26, 2021

Conversation

eco-monk
Copy link
Contributor

@eco-monk eco-monk commented Oct 11, 2021

Fixes #8344

Fixes #8560

Widget design audit: Table Widget

Description

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Created a table, added data
  • Selected rows on a table, to see that the new colour is applied on the table row
  • Enabled multi-select
    • selected multiple rows
      • Verified that the checkboxes are updated with the latest svg and colours
      • Checkbox hover styles are also verified
    • selected all rows and verified that the header checkbox changes icon appropriately
    • unselected all rows and verified the header checkbox is automatically unchecked
  • Validate search text box style

Checklist:

  • My code follows the style guidelines of this project (checked with prettier)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (not necessary)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works (only css changes)
  • New and existing unit tests pass locally with my changes (no new errors generated)

Test coverage results 🧪

🟢 Total coverage has increased
// Code coverage diff between base branch:release and head branch: design-audit/table-widget 
Status File % Stmts % Branch % Funcs % Lines
🟢 total 54.81 (0.01) 36.53 (0.03) 33.47 (0.01) 55.34 (0.01)
🟢 app/client/src/components/ads/Dropdown.tsx 52.87 (0.31) 37.5 (0.56) 25 (0) 51.05 (0.35)
🔴 app/client/src/components/designSystems/appsmith/SearchComponent.tsx 75.86 (-1.41) 75 (8.33) 50 (-10) 75 (-1.19)
🟢 app/client/src/widgets/TableWidget/component/TableHeader.tsx 50 (1.28) 53.06 (2) 30 (0) 47.37 (1.42)
🟢 app/client/src/widgets/TableWidget/component/TableStyledWrappers.tsx 70.24 (0.83) 21.43 (0.16) 52.83 (0.98) 70.24 (0.83)
🟢 app/client/src/widgets/TableWidget/component/TableUtilities.tsx 37.72 (1.19) 22.43 (0) 15.38 (0.75) 35.95 (1.31)

@eco-monk eco-monk changed the title Design audit/table widget fix: table widget design Oct 11, 2021
@github-actions github-actions bot added the Bug Something isn't working label Oct 11, 2021
@eco-monk eco-monk marked this pull request as ready for review October 11, 2021 16:41
@eco-monk eco-monk self-assigned this Oct 11, 2021
@appsmithorg appsmithorg deleted a comment from welcome bot Oct 12, 2021
@@ -44,6 +44,8 @@ export const Colors = {
GREEN: "#03B365",
JUNGLE_GREEN: "#24BA91",
JUNGLE_GREEN_DARKER: "#30A481",
FERN_GREEN: "#50AF6C",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have similar color used for select, @momcilo-appsmith why are we not using the same?

somangshu
somangshu previously approved these changes Oct 12, 2021
Copy link
Contributor

@somangshu somangshu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM, @momcilo-appsmith can check the DP once

@momcilo-appsmith
Copy link

@somangshu is right, we should have the hover for the whole row (including the checkbox div).
One additional thing is to change the cursor to the pointer on hover over the checkbox as well.

@eco-monk
Copy link
Contributor Author

eco-monk commented Oct 12, 2021

@somangshu is right, we should have the hover for the whole row (including the checkbox div). One additional thing is to change the cursor to the pointer on hover over the checkbox as well.

@momcilo-appsmith

  • Will change the cursor on checkbox.
  • Yes, changing the colour for whole row (on hover) would be a better way to handle this.
    Could you specify, which colour should be used for this. (Having it in the design would be good as well, for future references)

@momcilo-appsmith
Copy link

@eco-monk
Here you can find the selected color change: https://www.figma.com/file/w9CaXHF5iYcrLtiEbAHcFu/Original-Theme-Widgets-Library?node-id=622%3A6141
The hover color will be the same

@eco-monk eco-monk enabled auto-merge (squash) October 14, 2021 05:07
somangshu
somangshu previously approved these changes Oct 18, 2021
Copy link
Contributor

@somangshu somangshu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Needs manual testing to check more

@somangshu
Copy link
Contributor

/ok-to-test sha=74a0ae7

@eco-monk
Copy link
Contributor Author

eco-monk commented Oct 25, 2021

/ok-to-test sha=bffb3a0
https://github.com/appsmithorg/appsmith/runs/3993941915?check_suite_focus=true

@eco-monk
Copy link
Contributor Author

eco-monk commented Oct 25, 2021

/ok-to-test sha=df30a2d
https://github.com/appsmithorg/appsmith/actions/runs/1380080477

@eco-monk eco-monk requested review from jsartisan and removed request for somangshu and jsartisan October 25, 2021 08:17
@eco-monk
Copy link
Contributor Author

eco-monk commented Oct 25, 2021

/ok-to-test sha=df30a2d
https://github.com/appsmithorg/appsmith/runs/3994733337?check_suite_focus=true

@somangshu
Copy link
Contributor

@eco-monk please move this to the QA pipeline if all reported issue are fixed

@eco-monk
Copy link
Contributor Author

@eco-monk please move this to the QA pipeline if all reported issue are fixed

@somangshu
The pipeline has some failures for some reason
https://github.com/appsmithorg/appsmith/runs/3994733337?check_suite_focus=true

The deploy preview works as expected though, Is this good enough to proceed?

@somangshu
Copy link
Contributor

Seems like a CI failure, You can go ahead and try re-running the test. Regardless this is not a blocker for the QA continue testing

@eco-monk
Copy link
Contributor Author

@YogeshJayaseelan, the search text box is updated with the latest design now
I have updated my comments regarding your observations too.

We can proceed with testing this now.

@eco-monk
Copy link
Contributor Author

eco-monk commented Oct 25, 2021

/ok-to-test sha=df30a2d
https://github.com/appsmithorg/appsmith/runs/3995694126?check_suite_focus=true

@eco-monk eco-monk requested review from somangshu, ashit-rath and jsartisan and removed request for somangshu October 25, 2021 10:32
@eco-monk
Copy link
Contributor Author

eco-monk commented Oct 25, 2021

/ok-to-test sha=28ae5e7

https://github.com/appsmithorg/appsmith/runs/3999738855?check_suite_focus=true
https://github.com/appsmithorg/appsmith/actions/runs/1382036009

@eco-monk eco-monk added UI Builders Pod Issues that UI Builders face using appsmith and removed UI Improvement App Viewers Pod This label assigns issues to the app viewers pod UI Building Pod UI Builders Pod Issues that UI Builders face using appsmith labels Oct 26, 2021
@eco-monk eco-monk removed the request for review from jsartisan October 26, 2021 07:59
@YogeshJayaseelan
Copy link
Contributor

@eco-monk Tested this PR and selected and hover state matches figma design

@eco-monk eco-monk merged commit 9e55ca9 into release Oct 26, 2021
@eco-monk eco-monk deleted the design-audit/table-widget branch October 26, 2021 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Table Widget UI Builders Pod Issues that UI Builders face using appsmith
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Table widget's mouse over doesn't as expected [Bug] Widget design audit: Table Widget
5 participants