Select: Fix input cursor position so that it is at the start for single value selects (#41693)

* Select: Fix input cursor position so that it is at the start for single value selects * Fixing e2e tests * Fixes cursor issue * Fixing e2e tests * e2e fix * Select: ensure input always overlays singleValue, update pa11y config Co-authored-by: Ashley Harrison <ashley.harrison@grafana.com>
4 years ago · 9f4aa472cf
parent 130386f84b
commit 9f4aa472cf
13 changed files with 53 additions and 42 deletions
--- a/.pa11yci-pr.conf.js
+++ b/.pa11yci-pr.conf.js
@ -6,7 +6,9 @@ var config = {
    chromeLaunchConfig: {
      args: ['--no-sandbox'],
    },
-    hideElements: '#updateVersion',
+    // see https://github.com/grafana/grafana/pull/41693#issuecomment-979921463 for context
+    // on why we're ignoring singleValue/react-select-*-placeholder elements
+    hideElements: '#updateVersion, [class*="-singleValue"], [id^="react-select-"][id$="-placeholder"]',
  },

  urls: [
--- a/.pa11yci.conf.js
+++ b/.pa11yci.conf.js
@ -6,7 +6,9 @@ var config = {
    chromeLaunchConfig: {
      args: ['--no-sandbox'],
    },
-    hideElements: '#updateVersion',
+    // see https://github.com/grafana/grafana/pull/41693#issuecomment-979921463 for context
+    // on why we're ignoring singleValue/react-select-*-placeholder elements
+    hideElements: '#updateVersion, [class*="-singleValue"], [id^="react-select-"][id$="-placeholder"]',
  },

  urls: [
--- a/e2e/dashboards-suite/dashboard-time-zone.spec.ts
+++ b/e2e/dashboards-suite/dashboard-time-zone.spec.ts
@ -53,7 +53,7 @@ e2e.scenario({
    e2e.components.TimeZonePicker.containerV2()
      .should('be.visible')
      .within(() => {
-        e2e.components.Select.singleValue().should('be.visible').should('have.text', 'Coordinated Universal Time');
+        e2e.components.Select.singleValue().should('have.text', 'Coordinated Universal Time');
        e2e.components.Select.input().should('be.visible').click();
      });

--- a/e2e/smoke-tests-suite/variables/new-query-variable.ts
+++ b/e2e/smoke-tests-suite/variables/new-query-variable.ts
@ -18,7 +18,7 @@ describe('Variables - Add variable', () => {
    e2e.pages.Dashboard.Settings.Variables.Edit.General.generalTypeSelect()
      .should('be.visible')
      .within((select) => {
-        e2e.components.Select.singleValue().should('be.visible').should('have.text', 'Query');
+        e2e.components.Select.singleValue().should('have.text', 'Query');
      });
    e2e.pages.Dashboard.Settings.Variables.Edit.General.generalLabelInput()
      .should('be.visible')
@ -82,7 +82,7 @@ describe('Variables - Add variable', () => {

    e2e().get('#Description').should('be.visible').clear().type('a description');

-    e2e.components.DataSourcePicker.inputV2().should('be.visible').type('gdev-testdata{enter}');
+    e2e.components.DataSourcePicker.container().should('be.visible').type('gdev-testdata{enter}');

    e2e.pages.Dashboard.Settings.Variables.Edit.QueryVariable.queryOptionsQueryInput()
      .should('be.visible')
@ -132,7 +132,7 @@ describe('Variables - Add variable', () => {

    e2e().get('#Description').should('be.visible').clear().type('a description');

-    e2e.components.DataSourcePicker.inputV2().should('be.visible').type('gdev-testdata{enter}');
+    e2e.components.DataSourcePicker.container().type('gdev-testdata{enter}');

    e2e.pages.Dashboard.Settings.Variables.Edit.QueryVariable.queryOptionsQueryInput()
      .should('be.visible')
--- a/e2e/various-suite/exemplars.spec.ts
+++ b/e2e/various-suite/exemplars.spec.ts
@ -44,7 +44,7 @@ describe('Exemplars', () => {

    e2e.pages.Explore.visit();

-    e2e.components.DataSourcePicker.input().should('be.visible').click();
+    e2e.components.DataSourcePicker.container().should('be.visible').click();
    e2e().contains(dataSourceName).scrollIntoView().should('be.visible').click();

    // we need to wait for the query-field being lazy-loaded, in two steps:
--- a/e2e/various-suite/query-editor.spec.ts
+++ b/e2e/various-suite/query-editor.spec.ts
@ -8,7 +8,7 @@ e2e.scenario({
  skipScenario: false,
  scenario: () => {
    e2e.pages.Explore.visit();
-    e2e.components.DataSourcePicker.inputV2().should('be.visible').click();
+    e2e.components.DataSourcePicker.container().should('be.visible').click();

    cy.contains('gdev-prometheus').scrollIntoView().should('be.visible').click();
    const queryText = 'http_requests_total';
--- a/e2e/various-suite/trace-view-scrolling.spec.ts
+++ b/e2e/various-suite/trace-view-scrolling.spec.ts
@ -11,9 +11,7 @@ describe('Trace view', () => {

    e2e.pages.Explore.visit();

-    e2e.components.DataSourcePicker.inputV2().should('be.visible').click();
-
-    e2e().contains('gdev-jaeger').scrollIntoView().should('be.visible').click();
+    e2e.components.DataSourcePicker.container().should('be.visible').type('gdev-jaeger{enter}');

    e2e.components.QueryField.container().should('be.visible').type('long-trace');

--- a/packages/grafana-e2e/src/flows/importDashboard.ts
+++ b/packages/grafana-e2e/src/flows/importDashboard.ts
@ -50,7 +50,7 @@ export const importDashboard = (dashboardToImport: Dashboard, queryTimeout?: num
    e2e.components.Panels.Panel.title(panel.title).should('be.visible').click();
    e2e.components.Panels.Panel.headerItems('Inspect').should('be.visible').click();
    e2e.components.Tab.title('JSON').should('be.visible').click();
-    e2e.components.PanelInspector.Json.content().should('be.visible').contains('Panel JSON').click();
+    e2e.components.PanelInspector.Json.content().should('be.visible').contains('Panel JSON').click({ force: true });
    e2e.components.Select.option().should('be.visible').contains('Data').click();

    // ensures that panel has loaded without knowingly hitting an error
--- a/packages/grafana-ui/src/components/Select/InputControl.tsx
+++ b/packages/grafana-ui/src/components/Select/InputControl.tsx
@ -25,15 +25,12 @@ const getInputControlStyles = stylesFactory(
        css`
          width: 100%;
          max-width: 100%;
-
          display: flex;
          flex-direction: row;
          align-items: center;
          flex-wrap: wrap;
          justify-content: space-between;
-
          padding-right: 0;
-
          position: relative;
          box-sizing: border-box;
        `,
--- a/packages/grafana-ui/src/components/Select/SelectBase.tsx
+++ b/packages/grafana-ui/src/components/Select/SelectBase.tsx
@ -260,7 +260,7 @@ export function SelectBase<T>({
                  css`
                    display: inline-block;
                    color: ${theme.colors.text.disabled};
-
+                    grid-area: 1 / 1 / 2 / 3;
                    box-sizing: border-box;
                    line-height: 1;
                    white-space: nowrap;
--- a/packages/grafana-ui/src/components/Select/SingleValue.tsx
+++ b/packages/grafana-ui/src/components/Select/SingleValue.tsx
@ -18,8 +18,9 @@ const getStyles = (theme: GrafanaTheme2) => {
    text-overflow: ellipsis;
    box-sizing: border-box;
    max-width: 100%;
+    grid-area: 1 / 1 / 2 / 3;
  `;
-  const container = css`
+  const spinnerWrapper = css`
    width: 16px;
    height: 16px;
    display: inline-block;
@ -29,7 +30,7 @@ const getStyles = (theme: GrafanaTheme2) => {
    overflow: hidden;
  `;

-  const item = css`
+  const spinnerIcon = css`
    width: 100%;
    height: 100%;
    position: absolute;
@ -39,7 +40,7 @@ const getStyles = (theme: GrafanaTheme2) => {
    color: ${tinycolor(theme.colors.text.disabled).setAlpha(0.64).toString()};
  `;

-  return { singleValue, container, item, disabled };
+  return { singleValue, spinnerWrapper, spinnerIcon, disabled };
 };

 type StylesType = ReturnType<typeof getStyles>;
@ -52,36 +53,34 @@ export const SingleValue = <T extends unknown>(props: Props<T>) => {
  const loading = useDelayedSwitch(data.loading || false, { delay: 250, duration: 750 });

  return (
-    <components.SingleValue {...props}>
-      <div className={cx(styles.singleValue, isDisabled && styles.disabled)}>
-        {data.imgUrl ? (
-          <FadeWithImage
-            loading={loading}
-            imgUrl={data.imgUrl}
-            styles={styles}
-            alt={(data.label || data.value) as string}
-          />
-        ) : (
-          <SlideOutTransition horizontal size={16} visible={loading} duration={150}>
-            <div className={styles.container}>
-              <Spinner className={styles.item} inline />
-            </div>
-          </SlideOutTransition>
-        )}
-        {!data.hideText && children}
-      </div>
+    <components.SingleValue {...props} className={cx(styles.singleValue, isDisabled && styles.disabled)}>
+      {data.imgUrl ? (
+        <FadeWithImage
+          loading={loading}
+          imgUrl={data.imgUrl}
+          styles={styles}
+          alt={(data.label ?? data.value) as string}
+        />
+      ) : (
+        <SlideOutTransition horizontal size={16} visible={loading} duration={150}>
+          <div className={styles.spinnerWrapper}>
+            <Spinner className={styles.spinnerIcon} inline />
+          </div>
+        </SlideOutTransition>
+      )}
+      {!data.hideText && children}
    </components.SingleValue>
  );
 };

 const FadeWithImage = (props: { loading: boolean; imgUrl: string; styles: StylesType; alt?: string }) => {
  return (
-    <div className={props.styles.container}>
+    <div className={props.styles.spinnerWrapper}>
      <FadeTransition duration={150} visible={props.loading}>
-        <Spinner className={props.styles.item} inline />
+        <Spinner className={props.styles.spinnerIcon} inline />
      </FadeTransition>
      <FadeTransition duration={150} visible={!props.loading}>
-        <img className={props.styles.item} src={props.imgUrl} alt={props.alt} />
+        <img className={props.styles.spinnerIcon} src={props.imgUrl} alt={props.alt} />
      </FadeTransition>
    </div>
  );
--- a/packages/grafana-ui/src/components/Select/getSelectStyles.ts
+++ b/packages/grafana-ui/src/components/Select/getSelectStyles.ts
@ -66,6 +66,7 @@ export const getSelectStyles = stylesFactory((theme: GrafanaTheme2) => {
    singleValue: css`
      label: grafana-select-single-value;
      color: ${theme.components.input.text};
+      grid-area: 1 / 1 / 2 / 3;
      white-space: nowrap;
      overflow: hidden;
      text-overflow: ellipsis;
@ -75,7 +76,7 @@ export const getSelectStyles = stylesFactory((theme: GrafanaTheme2) => {
    valueContainer: css`
      label: grafana-select-value-container;
      align-items: center;
-      display: flex;
+      display: grid;
      position: relative;
      box-sizing: border-box;
      flex: 1 1 0%;
@ -85,6 +86,7 @@ export const getSelectStyles = stylesFactory((theme: GrafanaTheme2) => {
    valueContainerMulti: css`
      label: grafana-select-value-container-multi;
      flex-wrap: wrap;
+      display: flex;
    `,
    loadingMessage: css`
      label: grafana-select-loading-message;
--- a/packages/grafana-ui/src/components/Select/resetSelectStyles.ts
+++ b/packages/grafana-ui/src/components/Select/resetSelectStyles.ts
@ -1,3 +1,5 @@
+import { CSSObjectWithLabel } from 'react-select';
+
 export default function resetSelectStyles() {
  return {
    clearIndicator: () => ({}),
@ -8,7 +10,16 @@ export default function resetSelectStyles() {
    groupHeading: () => ({}),
    indicatorsContainer: () => ({}),
    indicatorSeparator: () => ({}),
-    input: () => ({}),
+    input: function (originalStyles: CSSObjectWithLabel) {
+      return {
+        ...originalStyles,
+        color: 'inherit',
+        margin: 0,
+        padding: 0,
+        // Set an explicit z-index here to ensure this element always overlays the singleValue
+        zIndex: 1,
+      };
+    },
    loadingIndicator: () => ({}),
    loadingMessage: () => ({}),
    menu: () => ({}),