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

docs: Add supported database logos #10368

Merged
merged 26 commits into from
Aug 3, 2020
Merged

docs: Add supported database logos #10368

merged 26 commits into from
Aug 3, 2020

Conversation

ceohockey60
Copy link
Contributor

SUMMARY

Adding logos of supported database solutions to make README more visual and easier to use.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

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

Adding logos of supported database solutions to make README more visual and easier to use
@ceohockey60 ceohockey60 changed the title Add supported database logos docs: Add supported database logos Jul 20, 2020
README.md Outdated
SQL toolkit that is compatible with most databases. Here are some of the major database solutions that are supported:

<p float="left">
<img src="https://i.ibb.co/qYyYdsr/redshift.png" alt="redshift" border="0" width="106" height="41" />
Copy link
Member

Choose a reason for hiding this comment

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

are these urls ever going to break? Can we add the images to the repo under the apache license?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a free imaging hosting site that appears good, so I used it to get this PR going, since I can't upload files to this repo.
If you can upload files to this repo and prefer that way, I can send you the images for you to upload, then I'll fix this PR with the relative links. Just let me know what you prefer! :)

Copy link
Member

Choose a reason for hiding this comment

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

Never heard of https://i.ibb.co, wouldn't rely on it.

Your images can be part of this PR (if you can PR you can add images).

@codecov-commenter
Copy link

codecov-commenter commented Jul 24, 2020

Codecov Report

Merging #10368 into master will decrease coverage by 5.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10368      +/-   ##
==========================================
- Coverage   70.63%   65.62%   -5.02%     
==========================================
  Files         601      604       +3     
  Lines       32338    32445     +107     
  Branches     3275     3285      +10     
==========================================
- Hits        22843    21293    -1550     
- Misses       9390    10966    +1576     
- Partials      105      186      +81     
Flag Coverage Δ
#cypress ?
#javascript 59.36% <ø> (-0.26%) ⬇️
#python 69.99% <ø> (+0.19%) ⬆️

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

Impacted Files Coverage Δ
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupFormatters.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/reducers/index.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupPluginsExtra.js 0.00% <0.00%> (-100.00%) ⬇️
... and 228 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 d92cb66...14ecb60. Read the comment docs.

@ceohockey60
Copy link
Contributor Author

Upload logos to repo and updated links. PTAL @etr2460 Thank you.

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

I'd recommend putting these images in a bit more generically named directory (maybe readme-assets or documentation-assets) but otherwise this lgtm

Although I would like confirmation from @mistercrunch (or someone else) that we can include these images within the repo under Apache's license?

@ktmud
Copy link
Member

ktmud commented Jul 27, 2020

I'd recommend putting them under superset-frontend/images/supported-databases and using relative paths:

<img src="supported-frontend/images/supported-databases/redshift.png" alt="redshift" border="0" width="106" height="41"/>

@etr2460
Copy link
Member

etr2460 commented Jul 27, 2020

@ktmud My thought was that we shouldn't include them in the package as they're not required for actually running Superset. It seems like keeping them seperate from the package would be better

@ktmud
Copy link
Member

ktmud commented Jul 27, 2020

I can see these images potentially being useful for the future doc site or when the Superset app adds a more graphic "add a datasource" page like Redash: https://redash.io/help/user-guide/getting-started#1-Add-A-Data-Source

Currently docs/_static/images points to superset-frontend anyway so I didn't think it was a big issue to add brand images in this existing folder.

@etr2460
Copy link
Member

etr2460 commented Jul 28, 2020

yikes, didn't realize we were packaging all the docs images in superset-frontend.... That's probably not a good idea in the long run.

However, I do see your point to there being value in including the images within the app. Moving to superset-frontend/images sounds good to me

@ceohockey60
Copy link
Contributor Author

Moved logos to superset-frontend/images and updated the path in README. PTAL @etr2460

README.md Outdated Show resolved Hide resolved
Co-authored-by: Jesse Yang <jesse.yang@airbnb.com>
@ktmud ktmud merged commit d6b7cae into apache:master Aug 3, 2020
Ofeknielsen pushed a commit to ofekisr/incubator-superset that referenced this pull request Oct 5, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.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 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants