feat(metadata): Metadata configuration UI #1951

igarashitm · 2023-06-12T20:35:15Z

BeansUI.mp4

codecov · 2023-06-12T21:12:17Z

Codecov Report

Merging #1951 (6087322) into main (f0a988f) will increase coverage by 3.78%.
The diff coverage is 92.36%.

@@            Coverage Diff             @@
##             main    #1951      +/-   ##
==========================================
+ Coverage   60.72%   64.50%   +3.78%     
==========================================
  Files          78       87       +9     
  Lines        2546     2820     +274     
  Branches      576      649      +73     
==========================================
+ Hits         1546     1819     +273     
  Misses        956      956              
- Partials       44       45       +1

Impacted Files	Coverage Δ
src/api/apiService.ts	`29.89% <75.00%> (+6.24%)`	⬆️
src/components/metadata/MetadataEditorBridge.tsx	`85.71% <85.71%> (ø)`
src/components/metadata/PropertyPlaceholderRow.tsx	`87.50% <87.50%> (ø)`
src/components/metadata/PropertyRow.tsx	`88.63% <88.63%> (ø)`
src/components/metadata/MetadataEditorModal.tsx	`92.30% <92.30%> (ø)`
src/components/metadata/PropertiesField.tsx	`92.98% <92.98%> (ø)`
src/components/metadata/MetadataToolbarItems.tsx	`96.15% <96.15%> (ø)`
src/components/metadata/ToopmostArrayTable.tsx	`96.77% <96.77%> (ø)`
src/components/KaotoToolbar.tsx	`54.95% <100.00%> (+3.13%)`	⬆️
src/components/Visualization.tsx	`74.52% <100.00%> (+0.99%)`	⬆️
... and 3 more

... and 3 files with indirect coverage changes

lordrip · 2023-06-13T11:05:10Z

There's an exception in the console, but I don't where is coming from yet.

igarashitm · 2023-06-13T11:53:47Z

@lordrip yes that's annoying, it appears with patternfly selectable table (left side). It adds <outout class="pf-screen-reader"> element next to every <tr>, right under the <tbody>. Even the patternfly official example shows the same error.

lordrip · 2023-06-13T10:34:02Z

src/components/SourceCodeEditor.tsx

@@ -57,7 +57,7 @@ export const SourceCodeEditor = (props: ISourceCodeEditor) => {
    } else {
      fetchTheSourceCode({ flows, properties, metadata }, settings);
    }
-  }, [flows, properties]);
+  }, [flows, properties, metadata]);


[Note for the future ourselves]: After #1927 is merged, we would need to add this property to the new location.

lordrip · 2023-06-13T10:35:47Z

src/components/index.ts

@@ -20,3 +20,4 @@ export * from './VisualizationControls';
 export * from './VisualizationStepViews';
 export * from './Visualization';
 export * from './VisualizationStep';
+export { MetadataToolbarItems } from './metadata/MetadataToolbarItems';


The content of this file is only exporting one single function, in which case, wouldn't be enough to do something like below?

Suggested change

export { MetadataToolbarItems } from './metadata/MetadataToolbarItems';

export * from './metadata/MetadataToolbarItems';

I'm curious if it's preferred in typescript? Generally the wildcard is not preferred in Java coding standards I know

In this case, it's ok as we're exposing tokens and not necessarily mean that they will be incorporated in the final bundle, is mostly convenience I would say.

src/components/metadata/AddPropertyButton.tsx

lordrip · 2023-06-13T12:31:04Z

src/components/metadata/MetadataEditorModal.test.tsx

+    expect(element).not.toBeInTheDocument();
+  });
+
+  test('editor works', async () => {


[friendly-suggestion]: This test might be more suited to an e2e test because if it fails, it might not be easy to determine the root cause as there are many expectations and also dependencies from the previous statement.

"Ideally" each test should have a couple expect() calls only. Notice how I quoted Ideally 😃

OK, I'll try to split the test... I'd like to keep these tests here with mock schemas, as e2e test will only work with the real schema which will be retrieved from backend in the future.

I think that it makes sense to keep the test with mock schemas as you're doing right now, I was suggesting more like preparing the data that the component will need, then rendering it, and then ensuring that it looks like you would like to.

Pretty much as you have it right now, but instead of interacting with the UI to reach the desired point, already provide that data to the component and then make the assertions on top of it.

So instead of testing everything, add/edit/delete in one place, test one action in one test, e.g. test add button and don't continue with the result of it, correct? I can do that.
Since this is an editor, just testing static rendering is not enough, rather need to verify actions work.

test one action in one test, e.g. test add button and don't continue with the result of it, correct? I can do that.

Exactly, just prepare the stage for the test, execute the action, and then do the evaluation. As AAA suggests.

Arrange, Act, and Assert.

src/components/metadata/MetadataEditorModal.tsx

src/store/FlowsStore.ts

lordrip · 2023-06-13T13:46:40Z

LGTM, just a couple of pending details:

The console error
The panel width changing when empty

lordrip · 2023-06-13T13:56:40Z

LGTM, just a couple of pending details:
* The console error

* The panel width changing when empty

It seems that the console error is coming from the @patternfly/react library
https://codesandbox.io/s/ji85yx

igarashitm · 2023-06-13T15:22:23Z

Discussed with @lordrip and now we think editing property name is important. Also the inline input for property value doesn't work with cursor key, so for now I'll remove the inline input and add "edit" button for the popover similar to "add new property".

igarashitm · 2023-06-13T18:04:43Z

Interestingly, while the following patternfly official doc also contains <output> under <tbody>, chrome doesn't show the warning in console
https://www.patternfly.org/v4/components/table/#composable-row-click-handler-hoverable--selected-rows
Although it does show once you open in the sandbox.

P.S. it's a known bug - patternfly/patternfly-react#7745

igarashitm · 2023-06-15T20:52:47Z

inlined property add/edit

2023-06-15_16-03-47.mp4

igarashitm · 2023-06-16T03:55:33Z

And fixed tests, should be ready now

Fixes: kaoto-archive#1722

* fix: Removed curly brackets for string literal attributes * fix: Prevent resizing * fix: Fixed cursor key behavior in property name/value input * fix: Inlined property add/edit with placeholder, removed popover * fix: Tests

sonarqubecloud · 2023-06-16T13:28:43Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

No Coverage information
4.0% Duplication

lordrip

LGTM, there are a couple of details that could be improved on the usability aspect but we could improve those along the way iteratively.

lordrip · 2023-06-16T14:17:02Z

src/components/metadata/MetadataToolbarItems.test.tsx

+jest.mock('@kaoto/api', () => {
+  return {
+    __esModule: true,
+    ...jest.requireActual('@kaoto/api'),


mmm I wonder what happened here, since we're not mocking any API 🤔

This is a "charm" I found at somewhere, without this the later spyOn didn't work - https://github.com/KaotoIO/kaoto-ui/pull/1951/files/608732269e75d09be304cd20e1d2468378a6ae04#diff-f37f26c5fb3f0d3c46bdfb42592db926f99f7d4db3bdc487958b5a7ac1c8c273R16
I barely remenber the post says __esModule: true is the key, not sure what it means 😅

igarashitm requested a review from a team June 12, 2023 20:35

igarashitm force-pushed the 1722 branch 2 times, most recently from eb7f8fb to c666ebd Compare June 12, 2023 21:26

lordrip reviewed Jun 13, 2023

View reviewed changes

igarashitm marked this pull request as draft June 13, 2023 15:22

igarashitm force-pushed the 1722 branch 3 times, most recently from 72524de to 11eb9f1 Compare June 15, 2023 20:21

igarashitm force-pushed the 1722 branch 2 times, most recently from 23d5545 to d656f43 Compare June 16, 2023 03:54

igarashitm marked this pull request as ready for review June 16, 2023 03:54

igarashitm added 2 commits June 16, 2023 09:27

feat(metadata): Metadata configuration UI

4400fb2

Fixes: kaoto-archive#1722

feat(metadata): Metadata configuration UI

6087322

* fix: Removed curly brackets for string literal attributes * fix: Prevent resizing * fix: Fixed cursor key behavior in property name/value input * fix: Inlined property add/edit with placeholder, removed popover * fix: Tests

igarashitm force-pushed the 1722 branch from d656f43 to 6087322 Compare June 16, 2023 13:27

lordrip approved these changes Jun 16, 2023

View reviewed changes

lordrip merged commit 3100ee1 into kaoto-archive:main Jun 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metadata): Metadata configuration UI #1951

feat(metadata): Metadata configuration UI #1951

igarashitm commented Jun 12, 2023 •

edited

Loading

codecov bot commented Jun 12, 2023 •

edited

Loading

lordrip commented Jun 13, 2023

igarashitm commented Jun 13, 2023

lordrip Jun 13, 2023

lordrip Jun 13, 2023

igarashitm Jun 13, 2023

lordrip Jun 15, 2023

lordrip Jun 13, 2023

igarashitm Jun 13, 2023

lordrip Jun 13, 2023

igarashitm Jun 13, 2023 •

edited

Loading

lordrip Jun 15, 2023

lordrip commented Jun 13, 2023

lordrip commented Jun 13, 2023

igarashitm commented Jun 13, 2023

igarashitm commented Jun 13, 2023 •

edited

Loading

igarashitm commented Jun 15, 2023 •

edited

Loading

igarashitm commented Jun 16, 2023

sonarqubecloud bot commented Jun 16, 2023

lordrip left a comment

lordrip Jun 16, 2023

igarashitm Jun 16, 2023

	export { MetadataToolbarItems } from './metadata/MetadataToolbarItems';
	export * from './metadata/MetadataToolbarItems';

feat(metadata): Metadata configuration UI #1951

feat(metadata): Metadata configuration UI #1951

Conversation

igarashitm commented Jun 12, 2023 • edited Loading

codecov bot commented Jun 12, 2023 • edited Loading

Codecov Report

lordrip commented Jun 13, 2023

igarashitm commented Jun 13, 2023

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

igarashitm Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordrip commented Jun 13, 2023

lordrip commented Jun 13, 2023

igarashitm commented Jun 13, 2023

igarashitm commented Jun 13, 2023 • edited Loading

igarashitm commented Jun 15, 2023 • edited Loading

igarashitm commented Jun 16, 2023

sonarqubecloud bot commented Jun 16, 2023

lordrip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igarashitm commented Jun 12, 2023 •

edited

Loading

codecov bot commented Jun 12, 2023 •

edited

Loading

igarashitm Jun 13, 2023 •

edited

Loading

igarashitm commented Jun 13, 2023 •

edited

Loading

igarashitm commented Jun 15, 2023 •

edited

Loading