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.

Sign up for GitHub

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

Jump to bottom

chore: Help user to find the input fields in the dataset editor #17824

Merged
merged 3 commits into from
Dec 22, 2021
Merged

chore: Help user to find the input fields in the dataset editor #17824

merged 3 commits into from
Dec 22, 2021

Conversation

lyndsiWilliams
Copy link
Member

@lyndsiWilliams lyndsiWilliams commented Dec 20, 2021 •
edited
Loading

SUMMARY

The following editable text areas are now more intuitive to their editable functionality:

  • The "Label" and "SQL expression" columns in the Metrics tab
  • The "Column" column in the Columns tab

I changed the EditableTitle components into TextControl/TextAreaControl components where applicable. This leaves the input box borders showing at all times instead of just when the user clicks the area.

BEFORE/AFTER SCREENSHOTS

BEFORE:

  • Metrics tab
    metricsTabBefore
  • Columns tab
    columnsTabBefore

AFTER:

  • Metrics tab
    metricsTabAfter
  • Columns tab
    verticallyAlignedRows

TESTING INSTRUCTIONS

  • Observe the described changes in the Edit Dataset model under the "Metrics" and "Columns" tabs.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@lyndsiWilliams
Updated fields in Metrics and Calculated Columns tabs inside dataset …
1f75105 
…editor, deleted label in expanded Metrics tab
@betodealmeida
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@betodealmeida Ephemeral environment spinning up at http://52.27.90.65:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@codecov
Copy link

codecov bot commented Dec 20, 2021 •
edited
Loading

Codecov Report

Merging #17824 (d9a2fad) into master (d9e9c3a) will decrease coverage by 1.69%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17824      +/-   ##
==========================================
- Coverage   68.49%   66.79%   -1.70%     
==========================================
  Files        1602     1608       +6     
  Lines       65350    64718     -632     
  Branches     6992     6863     -129     
==========================================
- Hits        44760    43228    -1532     
- Misses      18708    19625     +917     
+ Partials     1882     1865      -17
Flag Coverage Δ
javascript 53.76% <100.00%> (-3.35%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...end/src/components/Datasource/DatasourceEditor.jsx 68.93% <100.00%> (+0.07%) ⬆️
...ntend/src/dashboard/util/replaceUndefinedByNull.ts 0.00% <0.00%> (-100.00%) ⬇️
...d/plugins/preset-chart-xy/src/Line/legacy/index.ts 0.00% <0.00%> (-66.67%) ⬇️
...d/plugins/preset-chart-xy/src/ScatterPlot/index.ts 0.00% <0.00%> (-66.67%) ⬇️
...ns/preset-chart-xy/src/ScatterPlot/legacy/index.ts 0.00% <0.00%> (-66.67%) ⬇️
...lugins/preset-chart-xy/src/BoxPlot/legacy/index.ts 0.00% <0.00%> (-62.50%) ⬇️
...gins/legacy-plugin-chart-rose/src/controlPanel.tsx 50.00% <0.00%> (-50.00%) ⬇️
...gins/preset-chart-xy/src/BoxPlot/createMetadata.ts 0.00% <0.00%> (-50.00%) ⬇️
...ns/legacy-plugin-chart-horizon/src/controlPanel.ts 50.00% <0.00%> (-50.00%) ⬇️
...ns/legacy-plugin-chart-treemap/src/controlPanel.ts 50.00% <0.00%> (-50.00%) ⬇️
... and 569 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9e9c3a...d9a2fad. Read the comment docs.

betodealmeida
betodealmeida approved these changes Dec 20, 2021
View reviewed changes
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

So much better!

@eschutho eschutho added the hold:testing! On hold for testing label Dec 20, 2021
@eschutho
Copy link
Member

Looks great @lyndsiWilliams. Let's see if @yousoph or a designer have any feedback. Things look a little top aligned on the columns screen, but then the trashcan looks middle aligned for example, now that the row is deeper.

@lyndsiWilliams
Copy link
Member Author

Looks great @lyndsiWilliams. Let's see if @yousoph or a designer have any feedback. Things look a little top aligned on the columns screen, but then the trashcan looks middle aligned for example, now that the row is deeper.

Good call and I agree, maybe the "Sync columns from source" button can be moved to the bottom with the rest of the buttons to fix this? Although I'm not sure if it's important to keep that button at the top.

@ghost
Copy link

ghost commented Dec 20, 2021

@lyndsiWilliams Agree with @eschutho, let's vertically center align all the row elements. I think the "Sync columns from source" button should stay at the top, though, as there's already a lot of buttons at the bottom and will make it harder for the users to scan.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 22, 2021
@lyndsiWilliams
Copy link
Member Author

lyndsiWilliams commented Dec 22, 2021 •
edited
Loading

@jess-dillard @eschutho Great feedback, thank you! I've vertically aligned the rows in the columns tab in this commit and it looks much better. Here's a screenshot of the updated styling, which I've also added to the PR's summary.
verticallyAlignedRows

@lyndsiWilliams lyndsiWilliams merged commit d2ed1b7 into apache:master Dec 22, 2021
@lyndsiWilliams lyndsiWilliams deleted the lyndsi/improve-editor-fields branch December 22, 2021 18:21
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
@lyndsiWilliams @shcoderAlex
chore: Help user to find the input fields in the dataset editor (apac…
07ae801 
…he#17824)

* Updated fields in Metrics and Calculated Columns tabs inside dataset editor, deleted label in expanded Metrics tab

* Fix tests

* Vertically aligned rows in columns tab
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
@lyndsiWilliams @bwang221
chore: Help user to find the input fields in the dataset editor (apac…
a0b927a 
…he#17824)

* Updated fields in Metrics and Calculated Columns tabs inside dataset editor, deleted label in expanded Metrics tab

* Fix tests

* Vertically aligned rows in columns tab
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betodealmeida betodealmeida betodealmeida approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels hold:testing! On hold for testing preset-io size/M 🚢 1.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lyndsiWilliams @betodealmeida @eschutho @mistercrunch