Skip to content
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.

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

Migrate ConfigEditor and QueryEditor to the new form styling #211

Conversation

idastambuk
Copy link
Contributor

This migrates X-Ray Config and Query editors to the new form styling. The old styling is preserved and the new one is added under a feature toggle: awsDatasourcesNewFormStyling

Config Editor:

Screenshot 2023-10-26 at 4 00 24 PM Screenshot 2023-10-26 at 4 06 14 PM

  • Upgraded aws-sdk-react to use the new ConnectionConfig and passed the feature flag value

Query Editor

Screenshot 2023-10-26 at 4 00 39 PM
Screen.Recording.2023-10-26.at.4.06.40.PM.mov
  • changed the order of the input so that query type comes the first at the top. Since, depending on the query types, different form inputs are rendered, I found this looked better and less 'jumpy' above the query type selector.
  • uses Editor components from grafana/experimental
  • moved the old styling components to {componentName}Old so they can be easily removed when we remove the feature flag
  • tested in grafana 8.4.11, 9.4.7 and latest main

@idastambuk idastambuk requested a review from a team as a code owner October 26, 2023 14:49
@idastambuk idastambuk force-pushed the 629-migrate-x-ray-queryeditor-and-configeditor-to-new-form-styling branch from e7005a5 to 95b6b42 Compare October 26, 2023 14:51
Copy link
Member

@sarahzinger sarahzinger left a comment

Choose a reason for hiding this comment

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

Sounds neat, saw one thing that I think would be nice to fix if we can! Thanks for taking this on!

src/components/ConfigEditor/ConfigEditor.tsx Outdated Show resolved Hide resolved
const newFormStylingEnabled = config.featureToggles.awsDatasourcesNewFormStyling;

return (
<div className="width-30">
Copy link
Member

Choose a reason for hiding this comment

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

just curious if this div is still necessary with the new components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, since the new components adjust to the width of the container which we have to define at the top here. I don't think we have a fixed width requirement for our datasources (as long as it's not more that 578px, but that looks way too wide)

}

run('QueryEditorForm with awsDatasourcesNewFormStyling disabled');
grafanaRuntime.config.featureToggles.awsDatasourcesNewFormStyling = true;
Copy link
Member

Choose a reason for hiding this comment

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

So I think as a best practice we want to make sure that regardless of the configuration of the machine running these tests, we are properly testing both scenarios. We don't want to make an assumption for example that grafanaRuntime.config.featureToggles.awsDatasourcesNewFormStyling is false by default, because you might have it enabled on your machine. We also want to have some kind of clean up task so that there's no risk that this config could be set differently than expected in other tests down the line. Here's one way you could do that:

const originalValue = grafanaRuntime.config.featureToggles.awsDatasourcesNewFormStyling;

afterEach(() => {
    grafanaRuntime.config.featureToggles.awsDatasourcesNewFormStyling = originalValue
})

describe("it should render properly when awsDatasourcesNewFormStyling is enabled, () => {
    grafanaRuntime.config.featureToggles.awsDatasourcesNewFormStyling = true
    run()
})

describe("it should render properly when awsDatasourcesNewFormStyling is disabled, () => {
    grafanaRuntime.config.featureToggles.awsDatasourcesNewFormStyling = false
    run()
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it! Followed the same process like in Redshift and Athena if that sounds good :)

@idastambuk idastambuk merged commit 0a0ba70 into main Oct 31, 2023
1 check passed
@idastambuk idastambuk deleted the 629-migrate-x-ray-queryeditor-and-configeditor-to-new-form-styling branch October 31, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants