Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

style: Update text for SLL Tooltip #16993

Merged
merged 3 commits into from
Oct 11, 2021

Conversation

gabester78
Copy link
Contributor

@gabester78 gabester78 commented Oct 6, 2021

SUMMARY

Changed the SSL tooltip text to a general hint for all users.
https://trello.com/c/B4i4wtpq/29-update-ssl-tooltip-text

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen Shot 2021-10-06 at 3 36 51 PM

TESTING INSTRUCTIONS

  • From the Welcome screen navigate to Data > Databases
  • Click on "+ Database" button
  • Hover over SLL Tooltip at the bottom of the form

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

@gabester78 gabester78 changed the title Updated tooltip text to match Trello task. style(update text for SLL Tooltip) Oct 6, 2021
@gabester78 gabester78 marked this pull request as ready for review October 6, 2021 20:42
Copy link
Member

@lyndsiWilliams lyndsiWilliams left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@gabester78 gabester78 changed the title style(update text for SLL Tooltip) style: Update text for SLL Tooltip Oct 6, 2021
@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #16993 (18db25f) into master (3f6a24f) will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16993      +/-   ##
==========================================
- Coverage   76.90%   76.84%   -0.07%     
==========================================
  Files        1024     1026       +2     
  Lines       54931    54915      -16     
  Branches     7486     7478       -8     
==========================================
- Hits        42246    42197      -49     
- Misses      12437    12467      +30     
- Partials      248      251       +3     
Flag Coverage Δ
javascript 70.89% <ø> (-0.15%) ⬇️

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

Impacted Files Coverage Δ
.../database/DatabaseModal/DatabaseConnectionForm.tsx 48.71% <ø> (ø)
...rontend/src/components/Select/DeprecatedSelect.tsx 64.70% <0.00%> (-15.69%) ⬇️
superset-frontend/src/components/Select/styles.tsx 70.83% <0.00%> (-13.89%) ⬇️
.../src/explore/components/controls/SelectControl.jsx 78.68% <0.00%> (-10.70%) ⬇️
...ontend/src/dashboard/components/menu/HoverMenu.tsx 90.00% <0.00%> (-10.00%) ⬇️
.../explore/components/controls/TextControl/index.tsx 82.92% <0.00%> (-4.88%) ⬇️
...-frontend/src/dashboard/reducers/dashboardState.js 61.97% <0.00%> (-4.70%) ⬇️
...d/src/filters/components/Time/TimeFilterPlugin.tsx 83.33% <0.00%> (-3.34%) ⬇️
...nd/src/dashboard/util/activeAllDashboardFilters.ts 89.28% <0.00%> (-3.31%) ⬇️
...uperset-frontend/src/components/Menu/MenuRight.tsx 91.22% <0.00%> (-1.08%) ⬇️
... and 97 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 3f6a24f...18db25f. Read the comment docs.

@geido
Copy link
Member

geido commented Oct 7, 2021

@gabester78 Thanks for the contribution!

I believe the text might be misleading. Our documentation does not always have a reference to the SSL settings, see MySQL for instance. In other cases, such as Presto, you would just be redirected to the database list. I would rather change the text with something even more generic, like:

Enable SSL for increased security. Refer to the official documentation for more information.

@lyndsiWilliams
Copy link
Member

@gabester78 Thanks for the contribution!

I believe the text might be misleading. Our documentation does not always have a reference to the SSL settings, see MySQL for instance. In other cases, such as Presto, you would just be redirected to the database list. I would rather change the text with something even more generic, like:

Enable SSL for increased security. Refer to the official documentation for more information.

Hey Geido! This is the text that Sophie requested, but you bring up a good point if we don't have documentation for all database SSL settings. I think this would be better in that case, but I'd like to check and make sure it's alright to change - what do you think @yousoph ?

@yousoph
Copy link
Member

yousoph commented Oct 7, 2021

Thanks for the suggestion @geido, that's a good point. The text you suggested makes sense to me!

@gabester78
Copy link
Contributor Author

gabester78 commented Oct 7, 2021

Changed tooltip text per Geido's suggestion. I was having issues with the setup.cfg file being changed when trying to push up changes so I made the change directly in Github.

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

LGTM!

@betodealmeida betodealmeida merged commit 83a783d into apache:master Oct 11, 2021
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
* Updated tooltip text to match Trello task.

* Added graphlib

* Update DatabaseConnectionForm.tsx

Co-authored-by: Gabriel Romero <gabrielromero@Gabriels-MacBook-Pro.local>
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
* Updated tooltip text to match Trello task.

* Added graphlib

* Update DatabaseConnectionForm.tsx

Co-authored-by: Gabriel Romero <gabrielromero@Gabriels-MacBook-Pro.local>
@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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants