Skip to content

Conversation

mikaylathompson
Copy link
Collaborator

@mikaylathompson mikaylathompson commented Apr 30, 2025

Description

This PR basically gets all of the UI for the center column (transformations) functional.
You can now:

  • add new transformations
  • reorder transformations
  • resize the container for each transformation (e.g. collapse or expand it)
  • change the transformation's name
  • edit the content, with syntax highlighting, error catching, etc.
  • delete a transformation
  • apply formatting to a transformation
  • see a visual indicator of whether it's saved to local storage

The one thing you can't do in this PR is see the result of running them.

Issues Resolved

https://opensearch.atlassian.net/browse/MIGRATIONS-2491 (and beyond)

Testing

I haven't actually added any tests for this one because it does a minimal amount of changing state (and those state changes are already pretty well tested). Let me know if you think this leaves gaps.

Check List

  • New functionality includes testing
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

A few screenshots:

Basic overview:
Screenshot 2025-04-30 at 3 15 55 PM

Dragging to reorder:
Screenshot 2025-04-30 at 3 16 09 PM

Showing unsaved state indicator:
Screenshot 2025-04-30 at 3 16 31 PM

Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Copy link
Collaborator

@lewijacn lewijacn left a comment

Choose a reason for hiding this comment

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

Walked through this with Mikayla. We noticed one bug with the saving feature of the transformation that would cause inconsistent saving, but Mikayla is aware and working on fix for this. Other than this had no issue creating transformations, resizing them, moving them around, and deleting them 👍

@mikaylathompson
Copy link
Collaborator Author

Updated the editor internals to fix the saving error and show clearer messages about when the transformation was saved or if it wasn't saved
Screenshot 2025-05-01 at 12 26 22 PM
Screenshot 2025-05-01 at 1 00 30 PM

Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

@mikaylathompson Thanks for getting this out there, did a deep dive on the Ace integration and the react components. Called out a couple of files where I'd be curious for your thoughts are on testing.

const editorContent = editorRef.current?.editor.getValue();
const activeContent = editorContent ?? content;
// Skip update if transformation doesn't exist or if content is unchanged.
if (!transformation || activeContent === transformation.content) return;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!transformation || activeContent === transformation.content) return;
if (activeContent === transformation?.content) return;

Copy link
Member

Choose a reason for hiding this comment

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

Maybe worthwhile for some superficial UX tests for this component.

Copy link
Member

Choose a reason for hiding this comment

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

Moving items around on the board seems like it might be a good candidate for UX adding tests to confirm the behavior.

Copy link
Member

Choose a reason for hiding this comment

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

I'm curious if it is easy to see this break as you've been dev'ing on this component, trying to get an understanding of if we should have UX tests to verify callback/intervals being triggered correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oof. It's not a bad idea, but I'm truly dreading the implementation of that and a lot of this is in flux as I work on the execution step, so it feels like work that's going to change pretty dramatically. Can we postpone for now? I can create a task to track this work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Copy link
Collaborator

@lewijacn lewijacn left a comment

Choose a reason for hiding this comment

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

Bug I faced fixed after updates, thanks!

@mikaylathompson mikaylathompson merged commit 0f2c05b into opensearch-project:main May 2, 2025
58 checks passed
@mikaylathompson mikaylathompson deleted the playground-transformations branch May 2, 2025 18:49
Copy link

codecov bot commented May 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (b604f7e) to head (1a60b63).
Report is 8 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff      @@
##   main   #1489   +/-   ##
============================
============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

3 participants