-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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(widget-builder): Widget builder slideout #81444
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #81444 +/- ##
==========================================
- Coverage 80.37% 80.36% -0.01%
==========================================
Files 7226 7228 +2
Lines 319469 319530 +61
Branches 20762 20769 +7
==========================================
+ Hits 256775 256804 +29
- Misses 62304 62323 +19
- Partials 390 403 +13 |
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.
Small comments but lgtm 👍
|
||
export default WidgetBuilderSlideout; | ||
|
||
function WidgetBuilderSlideout({isOpen, onClose}: WidgetBuilderSlideoutProps) { |
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.
Can we rename this file to widgetBuilderSlideout.tsx
? The new
isn't really necessary
&:hover { | ||
color: ${p => p.theme.gray400}; | ||
} | ||
z-index: 100; |
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.
I was able to take this out and it worked fine, is it necessary?
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.
whoops no not really. This was a copy-paste mistake lolz
const SlideoutHeaderWrapper = styled('div')` | ||
padding: ${space(4)}; | ||
display: flex; | ||
justify-content: space-between; |
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 you lower the padding, add align-items: center
, and a bottom border, and remove the margin from the SlideoutTitle
, I think it ends up a bit closer to what the mockup is
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.
Ah yes I've already added this in my code for my next PR so it will be in there 👍
Bundle ReportChanges will increase total bundle size by 13.61kB (0.04%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
Just the bare bones of the widget builder UI. This adds the slide-out functionality and a placeholder widget. You can see it on any dashboard page with the url parameter `devBuilderUI=true`. It looks like this so far: ![image](https://github.com/user-attachments/assets/ae4d5792-3ed5-4c0a-ad81-f87af80be9c6)
Just the bare bones of the widget builder UI. This adds the slide-out functionality and a placeholder widget. You can see it on any dashboard page with the url parameter
devBuilderUI=true
. It looks like this so far: