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

chore: Tab title to be empty when creating a new tab #12773

Merged
merged 5 commits into from Feb 18, 2021
Merged

chore: Tab title to be empty when creating a new tab #12773

merged 5 commits into from Feb 18, 2021

Conversation

geido
Copy link
Member

@geido geido commented Jan 26, 2021

SUMMARY

Closes apache-superset/superset-roadmap#143 (roadmap item)

DEV.untitled.1.mp4

TEST PLAN

  1. Create a Dashboard
  2. Create a new tab
  3. Click on the tab title
  4. It should be empty with a placeholder "Tab title"

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@geido geido marked this pull request as ready for review January 26, 2021 18:40
@geido
Copy link
Member Author

geido commented Jan 26, 2021

@junlincc @rusackas

@codecov-io
Copy link

codecov-io commented Jan 26, 2021

Codecov Report

Merging #12773 (33e6465) into master (017f11f) will increase coverage by 3.79%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12773      +/-   ##
==========================================
+ Coverage   63.13%   66.93%   +3.79%     
==========================================
  Files        1022     1022              
  Lines       50032    50126      +94     
  Branches     4915     5201     +286     
==========================================
+ Hits        31587    33551    +1964     
+ Misses      18245    16444    -1801     
+ Partials      200      131      -69     
Flag Coverage Δ
cypress 50.98% <75.00%> (?)
javascript 61.72% <53.57%> (+0.04%) ⬆️
python 63.95% <ø> (-0.14%) ⬇️

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

Impacted Files Coverage Δ
...frontend/src/dashboard/util/newComponentFactory.js 100.00% <ø> (ø)
superset-frontend/src/components/EditableTitle.tsx 75.30% <71.42%> (+0.30%) ⬆️
...d/src/dashboard/components/gridComponents/Tabs.jsx 74.73% <84.61%> (+12.83%) ⬆️
...nd/src/dashboard/components/gridComponents/Tab.jsx 74.54% <100.00%> (+7.87%) ⬆️
superset/utils/celery.py 86.20% <0.00%> (-13.80%) ⬇️
superset/utils/cache.py 76.34% <0.00%> (-8.77%) ⬇️
superset/db_engine_specs/presto.py 82.25% <0.00%> (-6.28%) ⬇️
superset/db_engine_specs/sqlite.py 90.62% <0.00%> (-6.25%) ⬇️
superset/result_set.py 96.69% <0.00%> (-1.66%) ⬇️
...-frontend/src/datasource/ChangeDatasourceModal.tsx 84.14% <0.00%> (-1.04%) ⬇️
... and 207 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 017f11f...33e6465. Read the comment docs.

@geido geido changed the title chore: Tabs title empty when new tab chore: Tabs title to be empty when creating a new tab Jan 26, 2021
@geido geido changed the title chore: Tabs title to be empty when creating a new tab chore: Tab title to be empty when creating a new tab Jan 26, 2021
@junlincc
Copy link
Member

is it the same color we use in other placeholders in Superset?
@mihir174 do we have a standard for placeholder text color?

@junlincc junlincc added the need:design-review Requires input/approval from a Designer label Jan 27, 2021
@adam-stasiak
Copy link
Contributor

adam-stasiak commented Jan 27, 2021

We discussed behavior with @junlincc and some tweaks are needed:

  • placeholder should be 'Tab title'
  • when we add another tab we should switch to new tab in edit mode - that means user does not have to click anything to start typing - according to: Clicking the plus button should bring you into an empty input with "Tab title" as placeholder text, so you can start typing immediately.
    @mihir174 Could you think about this and confirm it is the thing we want. In other place in our application (SQL editor) when we add a new tab we are not in edition mode - but it is a little bit different case. While editing dashboard I assume we want to name our tab when we add this.

@geido
Copy link
Member Author

geido commented Jan 27, 2021

Hello @adam-stasiak the placeholder is 'Tab title' already. I assume you are referring to the default title 'New tab'. Do we want also that to be 'Tab title'?

@junlincc
Copy link
Member

@adam-stasiak thank you for testing!

@geido yes,

  1. once a new tab is created, the underline should goes under the new tab
  2. let's set both label and place holder "Tab title", it's probably more actionable
  3. I also notice there is tooltip when hovering on the input. i think changing tab name is pretty self-explanatory, so let's remove it.

Screen Shot 2021-01-27 at 11 42 26 PM

Thank you both! 🙏

@junlincc junlincc added the v1.1 label Jan 28, 2021
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jan 28, 2021
@geido
Copy link
Member Author

geido commented Jan 28, 2021

@adam-stasiak @junlincc I have updated the video to show the new behavior. Please have a look. Thanks!

Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

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

approving as product sign-off, thanks for the PR and review! @geido @nikolagigic

Screen.Recording.2021-02-03.at.11.24.47.PM.mov

good to go if the code looks good. @rusackas

@junlincc junlincc removed the need:design-review Requires input/approval from a Designer label Feb 4, 2021
@mihir174
Copy link
Contributor

mihir174 commented Feb 4, 2021

@junlincc no standard for this yet, can you pls add a des-system-revisit label?

@junlincc
Copy link
Member

junlincc commented Feb 5, 2021

no standard for this yet, can you pls add a des-system-revisit label?

added label! thanks Mihir for being on top of PRs! @mihir174

@simcha90
Copy link
Contributor

👍 Hi @geido thanks for PR it looks good changes, I just found some buggy behavior, can you look please:

Screen.Recording.2021-02-15.at.20.29.44.mov

@geido
Copy link
Member Author

geido commented Feb 16, 2021

Hello @simcha90 I can reproduce the same problem on master. This does not look related to my changes. I have created a separate issue #13158. Thanks for reporting it

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM Thanks for filing the issue discovered herein.

The defaultText/placeholder thing is my only remaining nit, but since that's basically a growing pain, I'll make a note, and we can circle back to sweep up later.

@rusackas rusackas closed this Feb 18, 2021
@rusackas rusackas reopened this Feb 18, 2021
@geido
Copy link
Member Author

geido commented Feb 18, 2021

Thanks @rusackas. I'll also take a note of that and we can tackle it as soon as we want to apply the enhancement globally.

@rusackas rusackas merged commit 409fc83 into apache:master Feb 18, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 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 size/M v1.1 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dasbhard] New dashboard tabs should go immediately into edit mode
9 participants