Select: Fix input cursor position so that it is at the start for sing…

…le value selects (grafana#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 <[email protected]>
james-deee · Nov 26, 2021 · 9f4aa47 · 9f4aa47
1 parent 130386f
commit 9f4aa47
Show file tree

Hide file tree

Showing 13 changed files with 53 additions and 42 deletions.
diff --git 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: [

diff --git 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: [

diff --git 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();
       });
 

diff --git 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')

diff --git 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:

diff --git 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';

diff --git 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');
 

diff --git 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

diff --git 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;
         `,

diff --git 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;

diff --git 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>
   );

diff --git 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;

diff --git 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: () => ({}),