-
Notifications
You must be signed in to change notification settings - Fork 14k
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: base tabbed modal for new database CRUD UI #10668
Conversation
superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
Outdated
Show resolved
Hide resolved
|
||
const { TabPane } = Tabs; | ||
|
||
const StyledIcon = styled(Icon)` |
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.
Wondering if this will be a common occurrence for the Icon (having text to the right of it). If so, it might make sense to add a prop to that component, handle the styling on that side, and just use the prop here.
superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
Outdated
Show resolved
Hide resolved
`; | ||
|
||
const StyledTabs = styled(Tabs)` | ||
margin-top: -18px; |
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 this to counteract some other padding/margin that should be removed instead?
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.
Yes, it was a hack to counteract the modal body default padding. Not sure if I want to change the modal padding since generally, it's nice to have. It just looks weird for the tabs in particular.
superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
Outdated
Show resolved
Hide resolved
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.
Looking good, I'm excited about a lot of these changes! Made some questions/comments/suggestions about small tweaks.
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.
Mostly looks good to me. Can we add some tests?
superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #10668 +/- ##
==========================================
+ Coverage 60.18% 64.25% +4.06%
==========================================
Files 783 789 +6
Lines 36934 37032 +98
Branches 3527 3535 +8
==========================================
+ Hits 22228 23794 +1566
+ Misses 14519 13126 -1393
+ Partials 187 112 -75
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
lgtm, thanks for adding some tests!
superset-frontend/spec/javascripts/views/CRUD/data/database/DatabaseModal_spec.jsx
Outdated
Show resolved
Hide resolved
I like the new tabs and modal, but we'll have to go back and migrate all of the react-bootstrap's instances of these components and point them to these new style wrappers (that's outside the scope of this PR). What color Is the active tab underline? looks like a dark blue. Shouldn't this be our brand-primary color? |
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.
Good progress! One question, but I may simply not be understanding the state of things :)
superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx
Outdated
Show resolved
Hide resolved
@mistercrunch yea that's a dark blue from our theme colors, as per the figma mocks. I can consult with design and possibly update it? |
492b450
to
036987f
Compare
6dd655b
to
f86c2ff
Compare
@riahk sorry about the confusion, stick with the Figma designs! Let's note that we now have 2 new component (tabs & modal) that don't fully meet the horizontal component process, so we'll need to:
|
@mistercrunch Yes, that's accurate. We'll track those as separate pieces of work. |
SUMMARY
DatabaseModal
opened from newDatabaseList
UIModal
tosrc/common/components
ENABLE_REACT_CRUD_VIEWS
andSIP_34_DATABASE_UI
config flags are turned onTabbed sections will be built in later PRs.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION