michael-s-molina commented on code in PR #22474:
URL: https://github.com/apache/superset/pull/22474#discussion_r1065885973
##########
superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx:
##########
@@ -138,15 +206,15 @@ export default class WithPopoverMenu extends
React.PureComponent<
>
{children}
{editMode && isFocused && (menuItems?.length ?? 0) > 0 && (
- <div className="popover-menu">
+ <PopoverMenuStyles className="popover-menu">
Review Comment:
```suggestion
<PopoverMenuStyles>
```
##########
superset-frontend/src/dashboard/components/gridComponents/new/DraggableNewComponent.jsx:
##########
@@ -35,6 +36,51 @@ const defaultProps = {
className: null,
};
+const NewComponent = styled.div`
+ display: flex;
+ flex-direction: row;
+ flex-wrap: nowrap;
+ align-items: center;
+ padding: ${({ theme }) => theme.gridUnit * 4}px;
+ background: ${({ theme }) => theme.colors.grayscale.light5};
+ cursor: move;
+
+ &:not(.static):hover {
+ background: ${({ theme }) => theme.colors.grayscale.light4};
+ }
+`;
+// Setting a displayName makes it easier to test with enzyme
+NewComponent.displayName = 'NewComponent';
+
+const NewComponentPlaceholder = styled.div`
+ position: relative;
+ background: ${({ theme }) => theme.colors.grayscale.light4};
+ width: ${({ theme }) => theme.gridUnit * 10}px;
+ height: ${({ theme }) => theme.gridUnit * 10}px;
+ margin-right: ${({ theme }) => theme.gridUnit * 4}px;
+ border: 1px solid ${({ theme }) => theme.colors.grayscale.light5};
+ display: flex;
+ align-items: center;
+ justify-content: center;
+ color: ${({ theme }) => theme.colors.text.label};
+ font-size: ${({ theme }) => theme.typography.sizes.xxl}px;
Review Comment:
Could you move theme to the top level and reuse it?
##########
superset-frontend/src/dashboard/components/gridComponents/new/DraggableNewComponent.test.jsx:
##########
@@ -73,7 +73,7 @@ describe('DraggableNewComponent', () => {
it('should render the passed label', () => {
const wrapper = setup();
- expect(wrapper.find('.new-component').text()).toBe(props.label);
+ expect(wrapper.find('NewComponent').text()).toBe(props.label);
Review Comment:
```suggestion
expect(
wrapper.find('[data-test="new-component"]').at(0).childAt(0).text(),
).toBe(props.label);
```
##########
superset-frontend/src/dashboard/components/gridComponents/new/DraggableNewComponent.jsx:
##########
@@ -35,6 +36,51 @@ const defaultProps = {
className: null,
};
+const NewComponent = styled.div`
+ display: flex;
+ flex-direction: row;
+ flex-wrap: nowrap;
+ align-items: center;
+ padding: ${({ theme }) => theme.gridUnit * 4}px;
+ background: ${({ theme }) => theme.colors.grayscale.light5};
+ cursor: move;
+
+ &:not(.static):hover {
+ background: ${({ theme }) => theme.colors.grayscale.light4};
+ }
+`;
+// Setting a displayName makes it easier to test with enzyme
+NewComponent.displayName = 'NewComponent';
Review Comment:
```suggestion
```
##########
superset-frontend/src/dashboard/components/resizable/ResizableContainer.jsx:
##########
@@ -80,6 +81,94 @@ const HANDLE_CLASSES = {
right: 'resizable-container-handle--right',
bottom: 'resizable-container-handle--bottom',
};
+
+const StyledResizable = styled(Resizable)`
+ ${({ theme }) => css`
+ &.resizable-container {
+ background-color: transparent;
+ position: relative;
+
+ /* re-resizable sets an empty div to 100% width and height, which doesn't
+ play well with many 100% height containers we need
+ */
Review Comment:
```suggestion
/* re-resizable sets an empty div to 100% width and height, which
doesn't
play well with many 100% height containers we need */
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
