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

feat: When editing the label/title in the Metrics popover, hitting Enter should save what you've typed #19898

Conversation

diegomedina248
Copy link
Contributor

SUMMARY

This PR adjusts the metrics popover in Explore to:

  • When editing the title, hitting enter should save the title as well
  • Adjust the size of the view & edit modes so that they take the same height and prevent the popover from changing positions

Additionally, took the opportunity to:

  • Refactor the file involved to use typescript
  • Refactor the tests to use react testing library instead, and added more tests scenarios

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not a visual change to see, so no before/after:

Screen.Recording.2022-04-29.at.11.41.53.mov

TESTING INSTRUCTIONS

Explore a chart, try to edit a metric and ensure the title gets saved on enter.

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

@diegomedina248 diegomedina248 changed the title feature: When editing the label/title in the Metrics popover, hitting Enter should save what you've typed feat: When editing the label/title in the Metrics popover, hitting Enter should save what you've typed Apr 29, 2022
@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #19898 (0baa9e6) into master (6f0d53e) will increase coverage by 0.00%.
The diff coverage is 84.37%.

❗ Current head 0baa9e6 differs from pull request most recent head 110e39a. Consider uploading reports for the commit 110e39a to get more accurate results

@@           Coverage Diff           @@
##           master   #19898   +/-   ##
=======================================
  Coverage   66.65%   66.66%           
=======================================
  Files        1729     1729           
  Lines       64910    64917    +7     
  Branches     6842     6843    +1     
=======================================
+ Hits        43268    43277    +9     
+ Misses      19893    19891    -2     
  Partials     1749     1749           
Flag Coverage Δ
javascript 51.59% <84.37%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...olumnSelectControl/DndColumnSelectPopoverTitle.jsx 8.00% <33.33%> (+3.45%) ⬆️
...rols/MetricControl/AdhocMetricEditPopoverTitle.tsx 89.65% <89.65%> (ø)

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 6f0d53e...110e39a. Read the comment docs.

.metric-edit-popover-label-input {
border-radius: @border-radius-large;
height: 30px;
height: 26px;
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to move both these classes to Emotion styling in the component? We're trying to whittle away at these LESS files so that we can use variables from the Superset theme everywhere (e.g. using the gridUnit and borderRadius keys from the theme)

@diegomedina248 diegomedina248 force-pushed the feature/when-editing-the-title-in-the-metrics-popover-hitting-enter-should-save-what-typed branch from 839cc8a to c168200 Compare June 3, 2022 19:00
@diegomedina248 diegomedina248 force-pushed the feature/when-editing-the-title-in-the-metrics-popover-hitting-enter-should-save-what-typed branch from c168200 to 110e39a Compare June 6, 2022 19:48
@rusackas rusackas merged commit 5bfc95e into apache:master Jun 8, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels Preset-Patch size/L 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants