268 migrate aws sdk react components to the new form styling #62

idastambuk · 2023-09-26T16:23:41Z

This is a part of migration to the new saga design system, namely using components from grafana/ui or the experimental components from /experimental. There are two parts to this, migrating query editor components and migrating config editor components, let me know if you'd rather have them split into two. Resource Selector cannot be split in the way I chose to do it, so I'm submitting a PR for both.

Thank you @aangelisc for all the support on this one!

Config Editor migration:

Before & After
(in Redshift Config editor)

What was done:

updated Grafana dependencies to 9.4.9. They will be updated as part of this epic to 10 afaik, since there needs to be some work done to migrate away from grafana/toolkit (deprecated in 10)

Connection Config:

replaced InlineFields with Fields from grafana/ui
removed fixed widths for the labels/fields from the config, as the width can be adjusted wherever the config is included. This means we no longer have to define a separate width especially for Auth
removed tooltips in favor of descriptions (as this is the recommendation). Some descriptions look a bit busy, but I chose consistency instead of having a combination of tooltips and descriptions.
Separated the auth part into 3 ConfigSubSections, Authentication, Assume Role and Additional Settings to break the form up a little bit. It made it look a bit busier, so if anyone feels strongly about removing this, I'd be happy to.
Added a Divider component to separate Config and datasource-specific fields. The Divider component is in grafana/ui>10. For this reason, I copied the logic from Azure Data Explorer (thanks @aangelisc!) that supports versions lower than 10. However, since our repo deps haven't been updated to v10, I had to comment the import for now. After we migrate away from grafana/toolkit, upgrade to Node 18 and upgrade grafana deps to 10, this can be included. Task here
removed the InlineInput component, as it will no longer be used in plugins

Query Editor components
(ResourceSelector, FillValueSelect, FormatSelect)

since ResourceSelector can be used in both Config editors and Query editors, whose fields are supposed to be wrapped in different components (Field as parent for config editor fields, and EditorField for query editor fields), I only left the Select component in the render. I'm still passing it the aria-label (for testing), id (for a11y).
I removed the "hidden" prop since we can hide the component from Field and EditorField props.
did the same thing for FormatSelect. However, since FillValueSelect wraps a few components, I wrapped those in . Theoretically, it can be used in the config editor, but it isn't currently, so I'm ok with leaving like this and improving it if we ever need it for something else
removed fixed widths for the labels/fields, like in the config
changed the labels a bit to make them more clear (taking suggestions for any I missed!)

This is what the query editor looks like in Redshift now. The changes from this PR are only with regards to the fields, not the layout. The layout changes are Redshift-specific.

aangelisc · 2023-10-03T12:15:25Z

Phenomenal work @idastambuk! Well done for getting everything in, I think it looks like a massive improvement 😊

iwysiu

LGTM! Up to you, but it's a big change so it may make sense to wait for someone besides me to also review it.

iwysiu · 2023-10-04T19:35:19Z

src/sql/ConfigEditor/ConfigSelect.tsx


 export interface ConfigSelectProps
  extends DataSourcePluginOptionsEditorProps<AwsAuthDataSourceJsonData, AwsAuthDataSourceSecureJsonData> {
  value: string;
+  // input id and aria-label necessary for accessibility attributes
+  id: string;
+  ['aria-label']: string;


Why is aria-label in brackets?

It's a way to define html element props that aren't camel case, like aria-label and data-testid so that typescript linter doesn't complain. I could have made the ariaLabel an 'official' component prop as well, of course. However, I replaced it in the final version with just label props.

iwysiu · 2023-10-04T19:44:24Z

src/sql/ConfigEditor/ConfigSelect.test.tsx

-    const label = 'foo-id';
-    render(<ConfigSelect {...props} disabled={true} label={label} />);
-    const selectEl = screen.getByLabelText(label);
+    render(<ConfigSelect {...props} disabled={true} aria-label={props['aria-label']} />);


does the aria-label need to be set separately here or is it set by the props field already?

kevinwcyu · 2023-10-04T20:24:27Z

src/components/Divider.tsx

@@ -0,0 +1,19 @@
+// copied from Azure Data Explorer plugin since there is not Divider component in G<10.1
+import React from 'react';
+// import { Divider as GrafanaDivider} from '@grafana/ui';


wouldn't this throw an error if we are running the plugin in grafana < 10.1? my understanding is that the plugins rely on the runtime for the @grafana/ui import so if we run in grafana < 10.1 it would not find the Divider component.

also, i think it'd be good to remove the commented out code for now and leave it in a draft branch and then link it to the github issue for enabling this.

FWIW I tested this when I implemented and it didn't throw in G<10.1 but maybe worth someone else checking too 😊

kevinwcyu · 2023-10-04T20:27:05Z

src/components/utils/version.ts

@@ -0,0 +1,53 @@
+// copied from Azure Data Explorer plugin


it doesn't look like these are exported except for use by the divider component. can this file also be moved to a draft branch and then linked to the github issue for enabling the divider component?

Good point, removed!

sarahzinger · 2023-10-05T12:37:50Z

src/components/ConnectionConfig.tsx

+          >
+            <Select
+              id="authProvider"
+              aria-label="Authentication Provider"


I'm curious is the aria-label here necessary/recommended by our accessibility folks? I would guess that the label + htmlFor would do the job of labeling this component but I could be wrong, I know these Select components are very tricky sometimes

Ok so I tested the aria-labels with a screen reader and it looks like we need them on Select components, but not on the new Inputs. I would expect the aria labels be applied if we have an htmlFor and id, but they aren't getting. I will open a bug ticket for this in main grafana, but in the meantime I removed new Input aria-labels but keeping the Select ones. I didn't touch the old ones, since they will be removed later anyway. Good catch!

sarahzinger

Overall looks pretty cool!

A few ideas come to mind, feel free to think about them and do what you think is best!

it looks like a lot of changes and I don't think it would be too crazy to try using a feature flag, but I also respect the choice not to! There's a cost to everything, etc, etc.
I see we're adding a lot of aria-labels here primarily for testing purposes, but I think one of the benefits of these new components is that you don't need to use aria-labels (as long as we have a label and say what component that label is for) You can read more about some of the negatives of putting aria-labels on things that don't need aria-labels here if you'd like: https://github.com/grafana/grafana/blob/main/contribute/style-guides/e2e.md#aria-labels-vs-data-testid Or at least that's my understanding, I'm not an a11y expert, so I defer to whatever our a11y working group recommends!

Thanks for doing this very exciting!

sarahzinger · 2023-10-05T13:08:26Z

src/sql/ResourceSelector.test.tsx

    }
-    const selectEl = screen.getByLabelText(props.label);
+    const selectEl = screen.getByLabelText(props['aria-label']);


Imo it's a bit easier to understand what's happening in a test here to say:
const selectEl = screen.getByLabelText("the actual label");

but a bit nitpicky of me!

sarahzinger · 2023-10-11T16:23:04Z

src/components/ConnectionConfig.tsx

+  return (
+    <>
+      {props.newFormStylingEnabled ? (
+        <NewConnectionConfig


I'm getting into extremely nitpicky territory here so please don't feel the need to change the code, but it's been something I've been thinking about with my own code so I thought maybe I'd share:

It can sometimes be nice to think about adding changes in such a way that we are minimizing future changes, so as to feel more confidence when we make them and reduce cleanup.

For example whenever we end up removing this feature flag we're going to have lingering references to an unused prop newFormStylingEnabled and we're going to have a component that does nothing except render a component (so we'll probably have to remove the NewConnectionConfig file and put it back in here).

Another alternative could be to:

directly import the feature flag here (then you don't need to pass the feature flag prop at all)

Keep the new ConnectionConfig in the file called ConnectionConfig, and put all the old stuff in a new file OldConnectionConfig, so that eventually you just delete that file.

I agree! I tried to do that in the rest of the files that I hadn't done yet, but I kept this one, as the change to move to OldConnectionConfig now, will take more than putting things from NewConnectionConfig back. I don't think it's super worth it for this specific one right now

sarahzinger · 2023-10-11T16:24:34Z

src/components/NewConnectionConfig.tsx

+  assumeRoleInstructionsStyle,
+  ...props
+}: NewConnectionConfigProps) => {
+  const options = props.options;


just curious why we pull options here rather than use the spread up above?

We're passing props to the handlers, but we need options in there too. In a few other places in the file we just need options, so I don't necessarily need the spread and can use props.options but decided to leave the shorthand

idastambuk added 3 commits September 14, 2023 16:28

COnnection Config

03ca647

Migrate forms for 9.4

1d02707

Cleanup

6e22d34

idastambuk force-pushed the 268-migrate-aws-sdk-react-components-to-the-new-form-styling branch from 97d41b3 to 6e22d34 Compare September 27, 2023 09:51

idastambuk added 5 commits September 27, 2023 11:57

Remove imports

2e23cda

Migrate FillValueSelect, adjust RespurceSelector for QEditors, fix tests

e8ae256

Rephrase label

fb49c85

Move common components to separate dir

560cb94

Move common components to separate dir

45b49d0

A11y for labels

481d1bb

idastambuk marked this pull request as ready for review October 3, 2023 12:29

idastambuk requested a review from a team as a code owner October 3, 2023 12:29

idastambuk requested review from iwysiu and sarahzinger and removed request for a team October 3, 2023 12:29

idastambuk added 2 commits October 3, 2023 15:16

Shorten assume role description

119071d

Add id to format select props

db06fa6

idastambuk mentioned this pull request Oct 3, 2023

Query Editor and Config Editor: Migrate to new form styling grafana/redshift-datasource#253

Closed

iwysiu approved these changes Oct 4, 2023

View reviewed changes

kevinwcyu reviewed Oct 4, 2023

View reviewed changes

sarahzinger reviewed Oct 5, 2023

View reviewed changes

sarahzinger approved these changes Oct 5, 2023

View reviewed changes

idastambuk added 2 commits October 6, 2023 11:58

ConnectionConfig

a6b4b44

Query editor migration

18a6a31

idastambuk mentioned this pull request Oct 8, 2023

Migrate Query and config editors to new form styling under feature toggle grafana/redshift-datasource#255

Merged

idastambuk added 4 commits October 8, 2023 22:23

Improvements

a7717eb

Add feature toggle changes

30c3230

Merge fixes

0f66b66

Add hidden prop for old styling

c9735f9

sarahzinger approved these changes Oct 11, 2023

View reviewed changes

idastambuk added 4 commits October 23, 2023 17:18

Fix spread

5f18069

Merge conflicts

9ba1b77

Changed my mind

7272041

Prepare 0.3.0

20ec667

idastambuk merged commit 1764267 into main Oct 23, 2023

idastambuk deleted the 268-migrate-aws-sdk-react-components-to-the-new-form-styling branch October 23, 2023 16:35

idastambuk mentioned this pull request Nov 13, 2023

Add Divider component from grafana 10 #70

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

268 migrate aws sdk react components to the new form styling #62

268 migrate aws sdk react components to the new form styling #62

idastambuk commented Sep 26, 2023 •

edited

Loading

aangelisc commented Oct 3, 2023

iwysiu left a comment

iwysiu Oct 4, 2023

idastambuk Oct 8, 2023

iwysiu Oct 4, 2023

kevinwcyu Oct 4, 2023

aangelisc Oct 5, 2023

kevinwcyu Oct 4, 2023

idastambuk Oct 8, 2023

sarahzinger Oct 5, 2023

idastambuk Oct 8, 2023

sarahzinger left a comment

sarahzinger Oct 5, 2023

sarahzinger Oct 11, 2023

idastambuk Oct 23, 2023

sarahzinger Oct 11, 2023

idastambuk Oct 23, 2023

268 migrate aws sdk react components to the new form styling #62

268 migrate aws sdk react components to the new form styling #62

Conversation

idastambuk commented Sep 26, 2023 • edited Loading

Config Editor migration:

aangelisc commented Oct 3, 2023

iwysiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahzinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idastambuk commented Sep 26, 2023 •

edited

Loading