-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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: DBC-UI Globally available across the app 🌎 #18722
Conversation
Co-authored-by: Hugh A. Miles II <hughmil3s@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #18722 +/- ##
==========================================
+ Coverage 66.32% 66.67% +0.35%
==========================================
Files 1620 1634 +14
Lines 63087 65054 +1967
Branches 6372 6616 +244
==========================================
+ Hits 41840 43376 +1536
- Misses 19590 19967 +377
- Partials 1657 1711 +54
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
c0dc697
to
d161dea
Compare
c560ec0
to
969b96e
Compare
969b96e
to
b1aeaa4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick non-blocking question, otherwise this looks good to me!
@@ -144,9 +151,25 @@ const RightMenu = ({ | |||
{menu.label} | |||
</> | |||
); | |||
|
|||
const handleMenuSelection = (itemChose: any) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is itemChose
a complicated type or would this be able to be defined with more specific than any
?
<a href={item.url}> {item.label} </a> | ||
</Menu.Item> | ||
<> | ||
{idx === 2 && <Menu.Divider />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe for future-proofing this, we can say idx > 1
instead of comparing it to 2 specifically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we go above idx > 1, this would render after every menu item after the first 2 items. We just want it to render it once right after the db connection buttons. Thats the one of the main reason i split out the db connection buttons from the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see the complication. Again I would suggest separating the concerns between the data at the top and the rendering at the bottom. You could add the logic to the data with something like:
{
label: t('Connect Google Sheet'),
name: GlobalMenuDataOptions.GOOGLE_SHEETS,
perm: HAS_GSHEETS_INSTALLED,
},
'---------',
{
label: t('Upload a CSV'),
name: 'Upload a CSV',
url: '/csvtodatabaseview/form',
perm: CSV_EXTENSIONS,
},
and then you can render the divider if typeof item === 'string'
or make it an object with a type of "divider"...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something we can discuss later.
superset/views/base.py
Outdated
@@ -366,6 +368,10 @@ def common_bootstrap_payload() -> Dict[str, Any]: | |||
ReportRecipientType.EMAIL, | |||
] | |||
|
|||
# verify client has google sheets installed | |||
available_specs = get_available_engine_specs() | |||
frontend_config["SHOW_GLOBAL_GSHEETS"] = bool(available_specs[GSheetsEngineSpec]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump of an earlier comment.. can we name this something more generic like HAS_GSHEETS_INSTALLED
so that it's less tied to the client side functionality?
// if user has any of these roles the dropdown will appear | ||
const configMap = { | ||
'Upload a CSV': CSV_EXTENSIONS, | ||
'Upload a Columnar file': COLUMNAR_EXTENSIONS, | ||
'Upload Excel': EXCEL_EXTENSIONS, | ||
[GlobalMenuDataOptions.GOOGLE_SHEETS]: SHOW_GLOBAL_GSHEETS, | ||
[GlobalMenuDataOptions.DB_CONNECTION]: true, // todo(hugh): add permission check here for database creation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of a separate map here, how about we move these values into the upper dropdownItems map with a key of something like has_perm
? Then below instead of configMap[item.name] === true
you can say item.has_perm
9ae0edb
to
dbae5cf
Compare
dbae5cf
to
e2b0570
Compare
{idx === 2 && <Menu.Divider />} | ||
<Menu.Item key={item.name}> | ||
{item.url && <a href={item.url}> {item.label} </a>} | ||
{item.url === undefined && item.label} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, but instead of two lines can you combine these by making it a ternary statement?
@AAfghahi how does this look to you? |
This reverts commit 209e3f4.
This reverts commit 209e3f4.
SUMMARY
We've now have added
Connect Database
+Connect Google Sheets
to the new main nav. To allow users easier entry point for connecting their database or a gsheetBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Connecting DB via Postgres
saving_db.mov
Connecting GSheet
gsheets_global.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION