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

Chore: Remove deprecated components from dashboard import pages #83747

Merged
merged 4 commits into from Mar 4, 2024

Conversation

Clarity-89
Copy link
Contributor

What is this feature?

  • Move the Form component to core as there is some need for it, particularly in class based components
  • Remove deprecated form and layout components from dashboard import pages

Part of #81226

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@Clarity-89 Clarity-89 added the no-changelog Skip including change in changelog/release notes label Mar 1, 2024
@Clarity-89 Clarity-89 added this to the 11.0.x milestone Mar 1, 2024
@Clarity-89 Clarity-89 self-assigned this Mar 1, 2024
@Clarity-89 Clarity-89 requested review from tolzhabayev and a team as code owners March 1, 2024 07:28
@Clarity-89 Clarity-89 requested review from oscarkilhed and kaydelaney and removed request for a team March 1, 2024 07:28
maxWidth?: number | 'none';
}

export function Form<T extends FieldValues>({
Copy link
Contributor

Choose a reason for hiding this comment

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

So is the point of this move that the other one in grafana/ui is deprecated? Do we expect people to have new use cases for the use of this Form? Just wondering why the need to duplicate if we will still keep the other deprecated Form for the foreseeable future anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal is to remove it from grafana/ui, there's nothing wrong with the component itself, just its dependence on reack-hook-form. That's why I think it's fine to have it in core.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok that's fair! I guess my main question is what is the plan for deletion? Do we know how many places it is used outside of grafana repo? Just wondering how long we will have this duplicated code.

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's not really gonna be duplicated code, this component is already a bit different from the one in grafana/ui and should be used from now on. Deleting the component from grafana/ui will be a bit more challenging as we don't have good data where it can be used, plus deleting it might break some unmaintained plugins which are still used. I'd like to get rid of it with the Grafana 11 release, but that might prove not feasible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! Just wanted to make sure we had some plan to delete that one 👍

Copy link
Contributor

@JoaoSilvaGrafana JoaoSilvaGrafana left a comment

Choose a reason for hiding this comment

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

Lgtm!

@Clarity-89 Clarity-89 merged commit 9bd84b3 into main Mar 4, 2024
24 checks passed
@Clarity-89 Clarity-89 deleted the forms/db-import branch March 4, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants