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: update text for dbconn modal #20773

Merged
merged 5 commits into from
Aug 5, 2022

Conversation

pkdotson
Copy link
Member

@pkdotson pkdotson commented Jul 19, 2022

SUMMARY

This pr updates the dbconnection modal text after user successfully connects to database.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before
Screen Shot 2022-08-01 at 8 50 28 PM

after
Screen Shot 2022-08-01 at 8 32 19 PM

TESTING INSTRUCTIONS

Go to db connection modal and connect database. User should see updated text.

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

@codecov
Copy link

codecov bot commented Jul 19, 2022

Codecov Report

Merging #20773 (47a21c0) into master (460b213) will decrease coverage by 0.01%.
The diff coverage is 22.22%.

@@            Coverage Diff             @@
##           master   #20773      +/-   ##
==========================================
- Coverage   66.35%   66.33%   -0.02%     
==========================================
  Files        1766     1767       +1     
  Lines       67124    67165      +41     
  Branches     7129     7145      +16     
==========================================
+ Hits        44541    44555      +14     
- Misses      20759    20782      +23     
- Partials     1824     1828       +4     
Flag Coverage Δ
javascript 52.09% <22.22%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
...s/CRUD/data/database/DatabaseModal/ModalHeader.tsx 66.66% <ø> (ø)
...c/views/CRUD/data/database/DatabaseModal/index.tsx 31.52% <12.50%> (-0.23%) ⬇️
...c/views/CRUD/data/database/DatabaseModal/styles.ts 76.69% <100.00%> (+0.22%) ⬆️
...charts/src/Timeseries/Regular/Bar/controlPanel.tsx 31.25% <0.00%> (-4.47%) ⬇️
...dashboard/components/AddSliceCard/AddSliceCard.tsx 65.90% <0.00%> (-1.54%) ⬇️
...BigNumber/BigNumberWithTrendline/transformProps.ts 45.45% <0.00%> (-1.52%) ⬇️
...ntend/src/explore/components/ExploreChartPanel.jsx 67.60% <0.00%> (-1.41%) ⬇️
...gin-chart-echarts/src/Timeseries/transformProps.ts 54.65% <0.00%> (-1.31%) ⬇️
...mponents/controls/AnnotationLayerControl/index.jsx 9.67% <0.00%> (-0.50%) ⬇️
...frontend/src/SqlLab/components/ResultSet/index.tsx 50.69% <0.00%> (-0.35%) ⬇️
... and 14 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@eschutho
Copy link
Member

eschutho commented Aug 2, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022

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

@eschutho
Copy link
Member

eschutho commented Aug 2, 2022

A few more small design nits:
Can you change the line height on the subtext (Create a dataset to start visualizing...) to 17px per the mocks?
Maybe just check with design about the color of the buttons, too? It looks darker to me in the mocks.

Also, when I click the back button on that screen, I still see the buttons:
Superset

@yousoph
Copy link
Member

yousoph commented Aug 2, 2022

cc @jess-dillard if you have any feedback to add

@yousoph
Copy link
Member

yousoph commented Aug 2, 2022

  • The button text on the Create Dataset and Query Data in SQL Lab doesn't look centered - there's too much space on the right side of the text - can you please double check if you're using the secondary button component? The spacing should be correct on the component
  • Mocks say "begin visualizing" rather than "start visualizing" on the subheader text
  • Typo - "to to query your data" with an extra "to"
    (Button color looks ok)

I noticed that the X button and the close button on the Dataset creation modal don't work when I am linked to it from the database connection screen, but they work normally when you open the modal from the CRUD view:
image

When clicking either of the buttons from the database connection success screen, the header/subheader disappeared and buttons jumped to the top of the modal and a loading state showed up on the modal. The modal shouldn't be moving when the screen is refreshing.

@eschutho
Copy link
Member

eschutho commented Aug 3, 2022

@pkdotson, yes correct the font is 14px/17px for font size and line height. I think it's the line height that needs to be adjusted just a tweak on the Create a dataset to start visualizing... text. Thanks!

@pull-request-size pull-request-size bot added size/M and removed size/S labels Aug 3, 2022
Copy link
Member

@eschutho eschutho left a comment

Choose a reason for hiding this comment

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

just some style feedback

@pkdotson pkdotson requested a review from eschutho August 3, 2022 21:39
@pkdotson
Copy link
Member Author

pkdotson commented Aug 4, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

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

@pkdotson pkdotson merged commit e214e1a into apache:master Aug 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 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 size/M 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants