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

Transformations: Support enum field conversion #76410

Merged
merged 35 commits into from
Nov 16, 2023
Merged

Transformations: Support enum field conversion #76410

merged 35 commits into from
Nov 16, 2023

Conversation

nmarrs
Copy link
Contributor

@nmarrs nmarrs commented Oct 12, 2023

What is this feature?

This adds an enum config editor to the Convert field type transform so that users can customize + display their enum data in supported panels.

Updated UX

latest.UX.mov

Why do we need this feature?

We added the ability to render enum data a while back but did not uncover any way to actually configure fields into the enum field config format.

Users can convert their field types into an enum field today via the Convert field type transform (implemented via this PR), but have no ability to rearrange / customize the resulting enum configs

Who is this feature for?

All users that want to display enum data in their panels

Which issue(s) does this PR fix?:

Fixes #72073

Created a follow up issue to address issue where user can add multiple overriding conversions for the same field (which breaks enum rendering)

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.

@nmarrs nmarrs added area/transformations area/panel/timeseries The main time series Graph panel no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Oct 12, 2023
@nmarrs nmarrs added this to the 10.2.x milestone Oct 12, 2023
@nmarrs nmarrs self-assigned this Oct 12, 2023
@nmarrs nmarrs removed the no-backport Skip backport of PR label Oct 17, 2023
@nmarrs nmarrs marked this pull request as ready for review October 17, 2023 04:35
@nmarrs nmarrs requested a review from a team as a code owner October 17, 2023 04:35
@nmarrs nmarrs requested review from a team, codeincarnate and oscarkilhed and removed request for a team October 17, 2023 04:35
@nmarrs nmarrs changed the title [WIP] [POC] Transformations: Add enum config editor to convert field type transform Transformations: Add enum config editor to convert field type transform Oct 17, 2023
@nmarrs nmarrs added the no-backport Skip backport of PR label Oct 17, 2023
…uding removing state for tracking current editing index)
@nmarrs nmarrs modified the milestones: 10.2.x, 10.3.x Nov 1, 2023
@nmarrs
Copy link
Contributor Author

nmarrs commented Nov 1, 2023

@leeoniya this is (finally) ready for review 😅

@leeoniya
Copy link
Contributor

leeoniya commented Nov 3, 2023

works pretty well :)

found some bugs:

  1. i think the enum config is not reset when deleting and re-adding the transform? not sure if this is this specific transform or all transforms.
  2. the trash icon shifts while dragging (probably needs to be fixed in base component)
  3. some wonkiness around clicking out after item editing not always defocusing/saving the edits
enum-no-reset.mp4

you can actually save items in an unfilled state. we should probably prevent adding new items when we have an empty one waiting to be filled, and clean them out before saving?

image

then things start to get weird, maybe cause using values as react list keys and ending up with dupes?:

enum-snafu.mp4

@oscarkilhed
Copy link
Contributor

oscarkilhed commented Nov 7, 2023

you can actually save items in an unfilled state. we should probably prevent adding new items when we have an empty one waiting to be filled, and clean them out before saving?

Maybe when adding a new enum, the edit input field could already be open and focus set to it? If it loses focus with nothing added to it, it could be removed?

@nmarrs
Copy link
Contributor Author

nmarrs commented Nov 8, 2023

  1. i think the enum config is not reset when deleting and re-adding the transform? not sure if this is this specific transform or all transforms.

Testing this it seems that current behavior is expected (config is drawn from panel JSON and if panel json isn't saved / updated then transform config can persist)

transform.config.mov
  1. the trash icon shifts while dragging (probably needs to be fixed in base component)

This one is actually a little tricky 😬 Basically what is happening is when you drag table row it is being taken out of the table and thus the table is re-rendering to match the "new" table state where the column max-width is being updated dynamically (see here for context). The only ways around this is to either:

  1. have a static default width for table row value (i.e. 100px or something) Currently this makes longer inputs wrap but that probably should be ok 🤷‍♂️
  2. Add more complex state management for figuring out the max width and hard coding that into place (ew)

I'll default to 1) for time being 👍

  1. some wonkiness around clicking out after item editing not always defocusing/saving the edits

Fixed this - following suggestion from @oscarkilhed 💯

updated.UX.exp.mov

mappedIndex: number;
onChangeEnumValue: (index: number, value: string) => void;
onRemoveEnumRow: (index: number) => void;
checkIsEnumUniqueValue: (value: string) => boolean;
Copy link
Contributor

@leeoniya leeoniya Nov 14, 2023

Choose a reason for hiding this comment

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

should this be a passed-in prop or can we just directly use the checkIsEnumUniqueValue from https://github.com/grafana/grafana/pull/76410/files#diff-0f946700206cc388640fbe8e8df18c407f2a947e1b8bd5a0acca1b51dc91477aR102-R104?

dunno if you had specific plans for more than one pluggable strategy for uniqueness testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

main reason for checkIsEnumUniqueValue being passed as a prop is wanting to avoid sending the entire enumRows object as a prop to every instance of the EnumMappingRow component

Copy link
Contributor

@leeoniya leeoniya left a comment

Choose a reason for hiding this comment

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

some minor nits, but LGTM 👍

@nmarrs nmarrs changed the title Transformations: Add enum config editor to convert field type transform Transformations: Support enum field conversion Nov 16, 2023
@nmarrs nmarrs merged commit 7397f97 into main Nov 16, 2023
20 checks passed
@nmarrs nmarrs deleted the enum-transform branch November 16, 2023 17:44
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transformation: Improve enum transformation
4 participants