Skip to content

fix: control panel fields styling (#12236) #12326

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.

Sign up for GitHub

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

Merged
rusackas merged 1 commit into from
Jan 12, 2021
Merged

fix: control panel fields styling (#12236) #12326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions superset-frontend/cypress-base/cypress/integration/explore/advanced.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,9 @@ describe('Advanced analytics', () => {
.find('input[type=text]')
.type('28 days{enter}');

cy.get('[data-test=time_compare]').find('.Select__control').click();
cy.get('[data-test=time_compare]')
.find('input[type=text]')
.type('364 days{enter}');
cy.get('[data-test=time_compare]')
.find('.Select__multi-value__label')
.contains('364 days');
.type('1 year{enter}');

cy.get('button[data-test="run-query-button"]').click();
cy.wait('@postJson');
Expand All @@ -51,10 +47,13 @@ describe('Advanced analytics', () => {
chartSelector: 'svg',
});

cy.get('[data-test=time_compare]').within(() => {
cy.get('.Select__multi-value__label').contains('364 days');
cy.get('.Select__multi-value__label').contains('28 days');
});
cy.get('.panel-title').contains('Advanced Analytics').click();
cy.get('[data-test=time_compare]')
.find('.Select__multi-value__label')
.contains('28 days');
cy.get('[data-test=time_compare]')
.find('.Select__multi-value__label')
.contains('1 year');
});
});

Expand Down
12 changes: 6 additions & 6 deletions superset-frontend/spec/javascripts/explore/components/SelectControl_spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ describe('SelectControl', () => {
placeholder="add something"
/>,
);
expect(withMulti.html()).not.toContain('placeholder=');
expect(withMulti.html()).not.toContain('option(s');
});
});
describe('withSingleChoice', () => {
Expand All @@ -125,15 +125,15 @@ describe('SelectControl', () => {
placeholder="add something"
/>,
);
expect(singleChoice.html()).not.toContain('placeholder=');
expect(singleChoice.html()).not.toContain('option(s');
});
});
describe('default placeholder', () => {
it('does not show a placeholder if there are no options', () => {
const defaultPlaceholder = mount(
<SelectControl {...defaultProps} choices={[]} multi />,
);
expect(defaultPlaceholder.html()).not.toContain('placeholder=');
expect(defaultPlaceholder.html()).not.toContain('option(s');
});
});
describe('all choices selected', () => {
Expand All @@ -145,12 +145,12 @@ describe('SelectControl', () => {
value={['today', '1 year ago']}
/>,
);
expect(allChoicesSelected.html()).toContain('placeholder=""');
expect(allChoicesSelected.html()).not.toContain('option(s');
});
});
});
describe('when select is multi', () => {
it('renders the placeholder when a selection has been made', () => {
it('does not render the placeholder when a selection has been made', () => {
wrapper = mount(
<SelectControl
{...defaultProps}
Expand All @@ -159,7 +159,7 @@ describe('SelectControl', () => {
placeholder="add something"
/>,
);
expect(wrapper.html()).toContain('add something');
expect(wrapper.html()).not.toContain('add something');
});
it('shows numbers of options as a placeholder by default', () => {
wrapper = mount(<SelectControl {...defaultProps} multi />);
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/components/Label/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ const SupersetLabel = styled(BootstrapLabel)`
background-color: ${({ theme }) => theme.colors.grayscale.light3};
color: ${({ theme }) => theme.colors.grayscale.dark1};
border-color: ${({ theme, onClick }) =>
onClick ? theme.colors.grayscale.light1 : 'transparent'};
onClick ? theme.colors.grayscale.light2 : 'transparent'};
&:hover {
background-color: ${({ theme, onClick }) =>
onClick ? theme.colors.primary.light2 : theme.colors.grayscale.light3};
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/components/Select/SupersetStyledSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export type SupersetStyledSelectProps<
// additional props for easier usage or backward compatibility
labelKey?: string;
valueKey?: string;
assistiveText?: string;
multi?: boolean;
clearable?: boolean;
sortable?: boolean;
Expand Down
42 changes: 28 additions & 14 deletions superset-frontend/src/components/Select/styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export const defaultTheme: (
spacing: {
baseUnit: 3,
menuGutter: 0,
controlHeight: 28,
controlHeight: 34,
lineHeight: 19,
fontSize: 14,
minWidth: '7.5em', // just enough to display 'No options'
Expand Down Expand Up @@ -160,9 +160,9 @@ export const DEFAULT_STYLES: PartialStylesConfig = {
{ isFocused, menuIsOpen, theme: { borderRadius, colors } },
) => {
const isPseudoFocused = isFocused && !menuIsOpen;
let borderColor = '#ccc';
let borderColor = colors.grayBorder;
if (isPseudoFocused) {
borderColor = '#000';
borderColor = colors.grayBorderDark;
} else if (menuIsOpen) {
borderColor = `${colors.grayBorderDark} ${colors.grayBorder} ${colors.grayBorderLight}`;
}
Expand All @@ -181,6 +181,7 @@ export const DEFAULT_STYLES: PartialStylesConfig = {
box-shadow: 0 1px 0 rgba(0, 0, 0, 0.06);
}
flex-wrap: nowrap;
padding-left: 1px;
`,
];
},
Expand Down Expand Up @@ -312,9 +313,31 @@ const {
DropdownIndicator,
Option,
Input,
SelectContainer,
} = defaultComponents as Required<DeepNonNullable<SelectComponentsType>>;

export const DEFAULT_COMPONENTS: SelectComponentsType = {
SelectContainer: ({ children, ...props }) => {
const {
selectProps: { assistiveText },
} = props;
return (
<div>
<SelectContainer {...props}>{children}</SelectContainer>
{assistiveText && (
<span
css={(theme: SupersetTheme) => ({
marginLeft: 3,
fontSize: theme.typography.sizes.s,
color: theme.colors.grayscale.light1,
})}
>
{assistiveText}
</span>
)}
</div>
);
},
Option: ({ children, innerProps, data, ...props }) => (
<ClassNames>
{({ css }) => (
Expand Down Expand Up @@ -344,22 +367,13 @@ export const DEFAULT_COMPONENTS: SelectComponentsType = {
</DropdownIndicator>
),
Input: (props: InputProps) => {
const {
selectProps: { isMulti, value, placeholder },
getStyles,
} = props;
const isMultiWithValue = isMulti && Array.isArray(value) && !!value.length;
const { getStyles } = props;
return (
<Input
{...props}
placeholder={isMultiWithValue ? placeholder : undefined}
css={getStyles('input', props)}
autoComplete="chrome-off"
inputStyle={
isMultiWithValue
? { ...INPUT_TAG_BASE_STYLES, width: '100%' }
: INPUT_TAG_BASE_STYLES
}
inputStyle={INPUT_TAG_BASE_STYLES}
/>
);
},
Expand Down
9 changes: 3 additions & 6 deletions superset-frontend/src/explore/components/DatasourcePanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,6 @@ const DatasourceContainer = styled.div`
padding-left: ${({ theme }) => theme.gridUnit * 2}px;
padding-bottom: 0px;
}
.form-control.input-sm {
margin-bottom: ${({ theme }) => theme.gridUnit * 3}px;
}
.ant-collapse-item {
background-color: ${({ theme }) => theme.colors.grayscale.light4};
.anticon.anticon-right.ant-collapse-arrow > svg {
Expand Down Expand Up @@ -130,8 +127,8 @@ const DatasourceContainer = styled.div`
font-size: ${({ theme }) => theme.typography.sizes.s}px;
color: ${({ theme }) => theme.colors.grayscale.light1};
}
.form-control.input-sm {
width: calc(100% - ${({ theme }) => theme.gridUnit * 2}px);
.form-control.input-md {
width: calc(100% - ${({ theme }) => theme.gridUnit * 4}px);
margin: ${({ theme }) => theme.gridUnit * 2}px auto;
}
.type-label {
Expand Down Expand Up @@ -186,7 +183,7 @@ const DataSourcePanel = ({
<input
type="text"
onChange={search}
className="form-control input-sm"
className="form-control input-md"
placeholder={t('Search Metrics & Columns')}
/>
<div className="field-selections">
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/explore/components/ExploreViewContainer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,10 @@ const Styles = styled.div`
border-top: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
.explore-column {
display: flex;
flex: 0 0 ${({ theme }) => theme.gridUnit * 80}px;
flex: 0 0 ${({ theme }) => theme.gridUnit * 95}px;
flex-direction: column;
padding: ${({ theme }) => 2 * theme.gridUnit}px 0;
max-width: ${({ theme }) => theme.gridUnit * 80}px;
max-width: ${({ theme }) => theme.gridUnit * 95}px;
max-height: 100%;
}
.data-source-selection {
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/explore/components/OptionControls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export const HeaderContainer = styled.div`
export const LabelsContainer = styled.div`
padding: ${({ theme }) => theme.gridUnit}px;
border: solid 1px ${({ theme }) => theme.colors.grayscale.light2};
border-radius: 3px;
border-radius: ${({ theme }) => theme.gridUnit}px;
`;

export const AddControlLabel = styled.div`
Expand All @@ -99,7 +99,7 @@ export const AddControlLabel = styled.div`
font-size: ${({ theme }) => theme.typography.sizes.s}px;
color: ${({ theme }) => theme.colors.grayscale.light1};
border: dashed 1px ${({ theme }) => theme.colors.grayscale.light2};
border-radius: 3px;
border-radius: ${({ theme }) => theme.gridUnit}px;
cursor: pointer;

:hover {
Expand Down
20 changes: 12 additions & 8 deletions superset-frontend/src/explore/components/controls/SelectControl.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,13 +203,6 @@ export default class SelectControl extends React.PureComponent {
return remainingOptions;
}

createPlaceholder() {
const optionsRemaining = this.optionsRemaining();
const placeholder =
this.props.placeholder || t('%s option(s)', optionsRemaining);
return optionsRemaining ? placeholder : '';
}

createMetaSelectAllOption() {
const option = { label: 'Select All', meta: true };
option[this.props.valueKey] = 'Select All';
Expand All @@ -235,9 +228,19 @@ export default class SelectControl extends React.PureComponent {
valueKey,
valueRenderer,
} = this.props;
const placeholder = this.createPlaceholder();

const optionsRemaining = this.optionsRemaining();
const optionRemaingText = optionsRemaining
? t('%s option(s)', optionsRemaining)
: '';
const placeholder = this.props.placeholder || optionRemaingText;
const isMulti = this.props.isMulti || this.props.multi;

let assistiveText;
if (isMulti && optionsRemaining && Array.isArray(value) && !!value.length) {
assistiveText = optionRemaingText;
}

const selectProps = {
autoFocus,
clearable,
Expand All @@ -257,6 +260,7 @@ export default class SelectControl extends React.PureComponent {
optionRenderer,
options: this.state.options,
placeholder,
assistiveText,
promptTextCreator,
selectRef: this.getSelectRef,
value,
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/explore/components/controls/TextControl.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ export default class TextControl extends React.Component<
return (
<div>
<ControlHeader {...this.props} />
<FormGroup controlId={this.state.controlId} bsSize="small">
<FormGroup controlId={this.state.controlId} bsSize="medium">
<FormControl
type="text"
data-test="inline-name"
Expand Down
9 changes: 9 additions & 0 deletions superset-frontend/stylesheets/less/cosmo/bootswatch.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,10 @@ table,

// Forms ======================================================================

.form-control {
box-shadow: none;
}

.has-warning {
.help-block,
.control-label,
Expand Down Expand Up @@ -395,6 +399,11 @@ label {
font-size: 24px;
}

.list-group-item {
padding-top: 5px;
padding-bottom: 5px;
}

a.list-group-item {
&-success {
&.active {
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/stylesheets/less/cosmo/variables.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@
//
// ## Define common padding and border radius sizes and more. Values based on 14px text and 1.428 line-height (~20px to start).

@padding-base-vertical: 10px;
@padding-base-vertical: 6.5px;
Copy link
Member

@rusackas rusackas Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is part of the broader bootstrap theme, I'm a little worried this might cause some other regression somewhere else in the application. Have you done some kind of visual audit, or could we make this change in the relevant component(s) with Emotion styling?

Copy link
Member Author

@michael-s-molina michael-s-molina Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a visual audit of most screens and I didn't found any problem. It shouldn't cause any problem because this is a configurable variable of Bootstrap. To limit the scope of this change we need to track all components that use classes that depend on this variable and apply Emotion styles individually.

@padding-base-horizontal: 18px;

@padding-large-vertical: 18px;
Expand Down Expand Up @@ -152,7 +152,7 @@

@btn-default-color: @bs-gray;
@btn-default-bg: @lightest;
@btn-default-border: @bs-gray-light;
@btn-default-border: @gray-light;

@btn-success-color: @btn-primary-color;
@btn-success-bg: @brand-success;
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/stylesheets/less/variables.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@

@almost-black: #263238;
@gray-dark: #484848;
@gray-light: #cfd8dc;
@gray-light: #e0e0e0;
Copy link
Member

@rusackas rusackas Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to affect (apparently) at least 68 other things in the codebase. Is this a situation where we could use Emotion's styled to apply this color in a more specific place (using theme.colors.grayscale.light2)?

Copy link
Member Author

@michael-s-molina michael-s-molina Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that was the intention because #e0e0e0 is the official gray-light theme color.

@gray: #879399;
@gray-bg: #f7f7f7;
@gray-heading: #a3a3a3;
Expand Down