michael-s-molina commented on code in PR #22474:
URL: https://github.com/apache/superset/pull/22474#discussion_r1065054745
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -164,70 +178,259 @@ const StyledContent = styled.div<{
${({ fullSizeChartId }) => fullSizeChartId && `z-index: 101;`}
`;
+const DashboardContentWrapper = styled.div`
+ ${({ theme }) => css`
+ &.dashboard {
+ position: relative;
+ flex-grow: 1;
+ display: flex;
+ flex-direction: column;
+ height: 100%;
+
+ /* only top-level tabs have popover, give it more padding to match
header + tabs */
+ & > .with-popover-menu > .popover-menu {
+ left: ${theme.gridUnit * 6}px;
+ }
+
+ /* drop shadow for top-level tabs only */
+ & .dashboard-component-tabs {
+ box-shadow: 0 ${theme.gridUnit}px ${theme.gridUnit}px 0
+ ${addAlpha(
+ theme.colors.grayscale.dark2,
+ parseFloat(theme.opacity.light) / 100,
+ )};
+ padding-left: ${theme.gridUnit *
+ 2}px; /* note this is added to tab-level padding, to match header */
+ }
+
+ .dropdown-toggle.btn.btn-primary .caret {
+ color: ${theme.colors.grayscale.light5};
+ }
+
+ .background--transparent {
+ background-color: transparent;
+ }
+
+ .background--white {
+ background-color: ${theme.colors.grayscale.light5};
+ }
+ }
+ &.dashboard--editing {
+ .grid-row:after,
+ .dashboard-component-tabs > .hover-menu:hover + div:after {
+ border: 1px dashed transparent;
+ content: '';
+ position: absolute;
+ width: 100%;
+ height: 100%;
+ top: 0;
+ left: 0;
+ z-index: 1;
+ pointer-events: none;
+ }
+
+ .resizable-container {
+ & .dashboard-component-chart-holder {
+ .dashboard-chart {
+ .chart-container {
+ cursor: move;
+ opacity: 0.2;
+ }
+
+ .slice_container {
+ /* disable chart interactions in edit mode */
+ pointer-events: none;
+ }
+ }
+
+ &:hover .dashboard-chart .chart-container {
+ opacity: 0.7;
+ }
+ }
+
+ &:hover,
+ &.resizable-container--resizing:hover {
+ & > .dashboard-component-chart-holder:after {
+ border: 1px dashed ${theme.colors.primary.base};
+ }
+ }
+ }
+
+ .resizable-container--resizing:hover > .grid-row:after,
+ .hover-menu:hover + .grid-row:after,
+ .dashboard-component-tabs > .hover-menu:hover + div:after {
+ border: 1px dashed ${theme.colors.primary.base};
+ z-index: 2;
+ }
+
+ .grid-row:after,
+ .dashboard-component-tabs > .hover-menu + div:after {
+ border: 1px dashed ${theme.colors.grayscale.light2};
+ }
+
+ /* provide hit area in case row contents is edge to edge */
+ .dashboard-component-tabs-content {
+ .dragdroppable-row {
+ padding-top: ${theme.gridUnit * 4}px;
+ }
+
+ & > div:not(:last-child):not(.empty-droptarget) {
+ margin-bottom: ${theme.gridUnit * 4}px;
+ }
+ }
+
+ .dashboard-component-chart-holder {
+ &:after {
+ content: '';
+ position: absolute;
+ width: 100%;
+ height: 100%;
+ top: 0;
+ left: 0;
+ z-index: 1;
+ pointer-events: none;
+ border: 1px solid transparent;
+ }
+
+ &:hover:after {
+ border: 1px dashed ${theme.colors.primary.base};
+ z-index: 2;
+ }
+ }
+
+ .contract-trigger:before {
+ display: none;
+ }
+ }
+
+ & .dashboard-component-tabs-content {
+ & > div:not(:last-child):not(.empty-droptarget) {
+ margin-bottom: ${theme.gridUnit * 4}px;
+ }
+
+ & > .empty-droptarget {
+ position: absolute;
+ width: 100%;
+ }
+
+ & > .empty-droptarget:first-child {
+ height: ${theme.gridUnit * 4}px;
+ top: -2px;
+ z-index: 10;
+ }
+
+ & > .empty-droptarget:last-child {
+ height: ${theme.gridUnit * 3}px;
+ bottom: 0;
+ }
+ }
+
+ .empty-droptarget:first-child .drop-indicator--bottom {
+ top: ${theme.gridUnit * 6}px;
+ }
+ `}
+`;
+
const StyledDashboardContent = styled.div<{
dashboardFiltersOpen: boolean;
editMode: boolean;
nativeFiltersEnabled: boolean;
filterBarOrientation: FilterBarOrientation;
}>`
- display: flex;
- flex-direction: row;
- flex-wrap: nowrap;
- height: auto;
- flex: 1;
-
- .grid-container .dashboard-component-tabs {
- box-shadow: none;
- padding-left: 0;
- }
-
- .grid-container {
- /* without this, the grid will not get smaller upon toggling the builder
panel on */
- width: 0;
+ ${({
+ theme,
+ dashboardFiltersOpen,
+ editMode,
+ nativeFiltersEnabled,
+ filterBarOrientation,
+ }) => css`
+ display: flex;
+ flex-direction: row;
+ flex-wrap: nowrap;
+ height: auto;
flex: 1;
- position: relative;
- margin-top: ${({ theme }) => theme.gridUnit * 6}px;
- margin-right: ${({ theme }) => theme.gridUnit * 8}px;
- margin-bottom: ${({ theme }) => theme.gridUnit * 6}px;
- margin-left: ${({
- theme,
- dashboardFiltersOpen,
- editMode,
- nativeFiltersEnabled,
- filterBarOrientation,
- }) => {
- if (
- !dashboardFiltersOpen &&
- !editMode &&
- nativeFiltersEnabled &&
- filterBarOrientation !== FilterBarOrientation.HORIZONTAL
- ) {
- return 0;
- }
- return theme.gridUnit * 8;
- }}px;
- ${({ editMode, theme }) =>
- editMode &&
+ .grid-container .dashboard-component-tabs {
+ box-shadow: none;
+ padding-left: 0;
+ }
+
+ .grid-container {
+ /* without this, the grid will not get smaller upon toggling the builder
panel on */
+ width: 0;
+ flex: 1;
+ position: relative;
+ margin-top: ${theme.gridUnit * 6}px;
+ margin-right: ${theme.gridUnit * 8}px;
+ margin-bottom: ${theme.gridUnit * 6}px;
+ margin-left: ${!dashboardFiltersOpen &&
Review Comment:
Maybe receive `marginLeft` as the param and move this logic outside the
style definition?
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -134,7 +197,11 @@ const SliceHeader: FC<SliceHeaderProps> = ({
const exploreUrl =
`/explore/?dashboard_page_id=${dashboardPageId}&slice_id=${slice.slice_id}`;
return (
- <div className="chart-header" data-test="slice-header" ref={innerRef}>
+ <ChartHeaderStyles
+ className="chart-header"
Review Comment:
Can we remove the dependency of the `chart-header` class?
##########
superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.jsx:
##########
@@ -49,6 +49,270 @@ const propTypes = {
onCloseModal: PropTypes.func.isRequired,
};
+const ScopeContainer = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ height: 80%;
+ margin-right: ${theme.gridUnit * -6}px;
+ font-size: ${theme.typography.sizes.m}px;
+
+ & .nav.nav-tabs {
+ border: none;
+ }
+
+ & .filter-scope-body {
+ flex: 1;
+ max-height: calc(100% - ${theme.gridUnit * 32}px);
+
+ .filter-field-pane,
+ .filter-scope-pane {
+ overflow-y: scroll;
+ }
+ }
+
+ & .warning-message {
+ padding: ${theme.gridUnit * 6}px;
+ }
+ `}
+`;
+
+const ScopeBody = styled.div`
+ ${({ theme }) => css`
+ &.filter-scope-body {
+ flex: 1;
+ max-height: calc(100% - ${theme.gridUnit * 32}px);
+
+ .filter-field-pane,
+ .filter-scope-pane {
+ overflow-y: scroll;
Review Comment:
Shouldn't it be `auto`?
##########
superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.jsx:
##########
@@ -49,6 +49,270 @@ const propTypes = {
onCloseModal: PropTypes.func.isRequired,
};
+const ScopeContainer = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ height: 80%;
+ margin-right: ${theme.gridUnit * -6}px;
+ font-size: ${theme.typography.sizes.m}px;
+
+ & .nav.nav-tabs {
+ border: none;
+ }
+
+ & .filter-scope-body {
+ flex: 1;
+ max-height: calc(100% - ${theme.gridUnit * 32}px);
+
+ .filter-field-pane,
+ .filter-scope-pane {
+ overflow-y: scroll;
+ }
+ }
+
+ & .warning-message {
+ padding: ${theme.gridUnit * 6}px;
+ }
+ `}
+`;
+
+const ScopeBody = styled.div`
+ ${({ theme }) => css`
+ &.filter-scope-body {
+ flex: 1;
+ max-height: calc(100% - ${theme.gridUnit * 32}px);
+
+ .filter-field-pane,
+ .filter-scope-pane {
+ overflow-y: scroll;
+ }
+ }
+ `}
+`;
+
+const ScopeHeader = styled.div`
+ ${({ theme }) => css`
+ height: ${theme.gridUnit * 16}px;
+ border-bottom: 1px solid ${theme.colors.grayscale.light2};
+ padding-left: ${theme.gridUnit * 6}px;
+ margin-left: ${theme.gridUnit * -6}px;
+
+ h4 {
+ margin-top: 0;
+ }
+
+ .selected-fields {
+ margin: ${theme.gridUnit * 3}px 0 ${theme.gridUnit * 4}px;
+ visibility: hidden;
+
+ &.multi-edit-mode {
+ visibility: visible;
+ }
+
+ .selected-scopes {
+ padding-left: ${theme.gridUnit}px;
+ }
+ }
+ `}
+`;
+
+const ScopeSelector = styled.div`
+ ${({ theme }) => css`
+ &.filters-scope-selector {
+ display: flex;
+ flex-direction: row;
+ position: relative;
+ height: 100%;
+
+ a,
+ a:active,
+ a:hover {
+ color: inherit;
+ text-decoration: none;
+ }
+
+ .react-checkbox-tree .rct-icon.rct-icon-expand-all,
+ .react-checkbox-tree .rct-icon.rct-icon-collapse-all {
+ font-family: ${theme.typography.families.sansSerif};
+ font-size: ${theme.typography.sizes.m}px;
+ color: ${theme.colors.primary.base};
+
+ &::before {
+ content: '';
+ }
+
+ &:hover {
+ text-decoration: underline;
+ }
+
+ &:focus {
+ outline: none;
+ }
+ }
+
+ .filter-field-pane {
+ position: relative;
+ width: 40%;
+ padding: ${theme.gridUnit * 4}px;
+ padding-left: 0;
+ border-right: 1px solid ${theme.colors.grayscale.light2};
+
+ .filter-container {
+ label {
+ font-weight: ${theme.typography.weights.normal};
+ margin: 0 0 0 ${theme.gridUnit * 4}px;
+ word-break: break-all;
+ }
+ }
Review Comment:
```suggestion
.filter-container label {
font-weight: ${theme.typography.weights.normal};
margin: 0 0 0 ${theme.gridUnit * 4}px;
word-break: break-all;
}
```
##########
superset-frontend/src/dashboard/components/gridComponents/Column.jsx:
##########
@@ -58,6 +59,50 @@ const propTypes = {
const defaultProps = {};
+const ColumnStyles = styled.div`
+ &.grid-column {
+ width: 100%;
+ position: relative;
+
+ & > :not(.hover-menu):not(:last-child) {
+ margin-bottom: ${({ theme }) => theme.gridUnit * 4}px;
+ }
+ }
+
+ &.grid-column--empty {
+ min-height: ${({ theme }) => theme.gridUnit * 25}px;
+
+ &:before {
+ content: 'Empty column';
Review Comment:
We need to translate the text here.
##########
superset-frontend/src/dashboard/components/gridComponents/Header.jsx:
##########
@@ -55,6 +56,62 @@ const propTypes = {
const defaultProps = {};
+const HeaderStyles = styled.div`
+ font-weight: ${({ theme }) => theme.typography.weights.bold};
Review Comment:
Could you move `theme` to the top level and reuse it?
##########
superset-frontend/src/dashboard/components/gridComponents/Row.jsx:
##########
@@ -201,7 +234,7 @@ class Row extends React.PureComponent {
/>
</HoverMenu>
)}
- <div
+ <GridRow
className={cx(
'grid-row',
rowItems.length === 0 && 'grid-row--empty',
Review Comment:
```suggestion
<GridRow
className={cx(
rowItems.length === 0 && 'grid-row--empty',
```
##########
superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.jsx:
##########
@@ -496,28 +760,28 @@ export default class FilterScopeSelector extends
React.PureComponent {
const { showSelector } = this.state;
return (
- <div className="filter-scope-container">
- <div className="filter-scope-header">
+ <ScopeContainer className="filter-scope-container">
+ <ScopeHeader className="filter-scope-header">
Review Comment:
```suggestion
<ScopeHeader>
```
##########
superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.jsx:
##########
@@ -49,6 +49,270 @@ const propTypes = {
onCloseModal: PropTypes.func.isRequired,
};
+const ScopeContainer = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ height: 80%;
+ margin-right: ${theme.gridUnit * -6}px;
+ font-size: ${theme.typography.sizes.m}px;
+
+ & .nav.nav-tabs {
+ border: none;
+ }
+
+ & .filter-scope-body {
+ flex: 1;
+ max-height: calc(100% - ${theme.gridUnit * 32}px);
+
+ .filter-field-pane,
+ .filter-scope-pane {
+ overflow-y: scroll;
Review Comment:
Shouldn't it be `auto`?
##########
superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx:
##########
@@ -261,17 +274,17 @@ const ChartHolder: React.FC<ChartHolderProps> = ({
onResizeStop={onResizeStop}
editMode={editMode}
>
- <div
+ <FullSizeWrapper
Review Comment:
Another way of writing this logic is to compile the CSS and apply it only if
necessary:
```
const fullSizeStyle = css`
&& {
position: fixed;
z-index: 3000;
left: 0;
top: 0;
}
`;
<div
css={isFullSize ? fullSizeStyle : undefined}
>
```
##########
superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.jsx:
##########
@@ -496,28 +760,28 @@ export default class FilterScopeSelector extends
React.PureComponent {
const { showSelector } = this.state;
return (
- <div className="filter-scope-container">
- <div className="filter-scope-header">
+ <ScopeContainer className="filter-scope-container">
Review Comment:
```suggestion
<ScopeContainer>
```
##########
superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx:
##########
@@ -83,6 +83,35 @@ Click here to learn more about [markdown
formatting](https://bit.ly/1dQOfRK)`;
const MARKDOWN_ERROR_MESSAGE = t('This markdown component has an error.');
+const MarkdownStyles = styled.div`
+ &.dashboard-markdown {
+ overflow: hidden;
+
+ h4,
+ h5,
+ h6 {
+ font-weight: ${({ theme }) => theme.typography.weights.normal};
+ }
+
+ h5 {
+ color: ${({ theme }) => theme.colors.grayscale.base};
+ }
+
+ h6 {
+ font-size: ${({ theme }) => theme.typography.sizes.s}px;
Review Comment:
Could you move theme to the top level and reuse it?
##########
superset-frontend/src/dashboard/components/gridComponents/Divider.jsx:
##########
@@ -36,6 +37,29 @@ const propTypes = {
deleteComponent: PropTypes.func.isRequired,
};
+const DividerLine = styled.div`
+ width: 100%;
+ padding: ${({ theme }) => theme.gridUnit * 2}px 0; /* this is padding not
margin to enable a larger mouse target */
Review Comment:
Could you move `theme` to the top level and reuse it?
##########
superset-frontend/src/dashboard/components/gridComponents/Row.jsx:
##########
@@ -58,6 +58,39 @@ const propTypes = {
updateComponents: PropTypes.func.isRequired,
};
+const GridRow = styled.div`
+ position: relative;
+ display: flex;
+ flex-direction: row;
+ flex-wrap: nowrap;
+ align-items: flex-start;
+ width: 100%;
+ height: fit-content;
+
+ & > :not(:last-child):not(.hover-menu) {
+ margin-right: ${({ theme }) => theme.gridUnit * 4}px;
+ }
+
+ &.grid-row--empty {
+ /* this centers the empty note content */
+ align-items: center;
+ height: ${({ theme }) => theme.gridUnit * 25}px;
+
+ &:before {
+ position: absolute;
+ top: 0;
+ left: 0;
+ content: 'Empty row';
+ display: flex;
+ align-items: center;
+ justify-content: center;
+ width: 100%;
+ height: 100%;
+ color: ${({ theme }) => theme.colors.text.label};
Review Comment:
Could you move theme to the top level and reuse it?
--
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]
