Schema-item #974

ashwin-pc · 2023-08-17T01:39:00Z

Description

Adds SchemaItem as an experimental component to OUI

Issues Resolved

closes OuiSchemaListItem & OuiSchemaList #908

Check List

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.

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

KrooshalUX

👏 👏 👏 👏
Welcome to the world, SchemaItem!
🎉 👶 🎉

BSFishy

Looks good overall, just a few nits! If this needs to get in soon, I'd be fine with pushing this through and making most/all of my comments into followup issues since this component is marked as experimental

src-docs/src/views/schema/schema_group.tsx

BSFishy · 2023-08-17T02:31:25Z

src/components/schema/_schema_item.scss

+
+.ouiSchemaItem__icon {
+  align-self: flex-start;
+  margin: $ouiSize / 2 0; // To align with buttonIcon


this division will need to be surrounded by calc(), for sass upgrade (also makes it clearer where the calculation is taking place :P)

Suggested change

margin: $ouiSize / 2 0; // To align with buttonIcon

margin: calc($ouiSize / 2) 0; // To align with buttonIcon

This feels like a little bit of a hack? I think there are some existing approaches to align with buttons, although maybe I'm just thinking of https://oui.opensearch.org/1.0/#/navigation/button#empty-button

@joshuarrrr I know why you feel that way, because i felt it too when i was implementing it. Open to suggestions here and probably can be improved in a followup, but the root of the problem here is that ButtonIcons have an margin that we need to match and unfortunately this is the best way i could find to do it in both the compressed and uncompressed mode.

src/components/schema/_schema_item.scss

src/components/schema/schema_item.test.tsx

BSFishy · 2023-08-17T02:34:48Z

src/components/schema/schema_item.tsx

+  /** Whether the item is compressed. */
+  compressed?: boolean;
+  /** Whether the item is displayed with a panel. */
+  withPanel?: boolean;


Is there precedence for using with? I feel like a more common naming convention would be hasPanel

droppable uses withPanel

contextMenu uses hasPanel

pageBody uses panelled

So I don't think we have a consistent pattern today. Seeing that the current implementation is pure style (and doesn't actually use existing panel components or classes), I think I'd maybe choose withPanelStyling or something like that.

I'd personally avoid withPanelStyling since its unnecessarily long, but im okay with withPanel, hasPanel or even panelled. Whichever you want to align on. From a user's perspective, as long as it does what the docs say, i dont care as much what its called :)

src/components/schema/schema_item.tsx

src-docs/src/views/schema/schema.tsx

joshuarrrr

Looks good - mostly some questions around styling and alignment. The major thing missing is tests.

src/components/schema/schema_item.test.tsx

joshuarrrr · 2023-08-17T22:01:11Z

src/components/schema/schema_item.tsx

+  /** Whether the item is compressed. */
+  compressed?: boolean;
+  /** Whether the item is displayed with a panel. */
+  withPanel?: boolean;


droppable uses withPanel

contextMenu uses hasPanel

pageBody uses panelled

So I don't think we have a consistent pattern today. Seeing that the current implementation is pure style (and doesn't actually use existing panel components or classes), I think I'd maybe choose withPanelStyling or something like that.

joshuarrrr · 2023-08-17T22:15:57Z

src/components/schema/_schema_item.scss

+
+.ouiSchemaItem__icon {
+  align-self: flex-start;
+  margin: $ouiSize / 2 0; // To align with buttonIcon


This feels like a little bit of a hack? I think there are some existing approaches to align with buttons, although maybe I'm just thinking of https://oui.opensearch.org/1.0/#/navigation/button#empty-button

joshuarrrr · 2023-08-17T22:17:11Z

src/components/schema/_schema_item.scss

+  .ouiButtonIcon {
+    opacity: 0;
+    transition: opacity $ouiAnimSpeedNormal $ouiAnimSlightResistance;
+
+    &:focus {
+      opacity: 1;
+    }
+  }


Is there not an existing mixin to use for this?

I copied this over from ListGroup. I dont think it used one, but if there is one I will update it

joshuarrrr · 2023-08-17T22:19:40Z

src/components/schema/_schema_item.scss

+  @include ouiBottomShadowSmall;
+  border: $ouiBorderThin;
+
+  &:hover {
+    background-color: tintOrShade($ouiColorLightestShade, 50%, 20%);
+  }


The naming of this seems a little fishy. I would expect the component prop to just wrap itself with https://oui.opensearch.org/1.0/#/layout/panel. This is just adding border and a hover background color. Is it intended to actually leverage/mimic existing panel styles or is it just a different styling for this component?

It is, but its style is different from a normal panel. Based on @KrooshalUX's mock in the parent issue, the colors here come from other similar components and they do it this way. We can probably reuse them, but given that OUI components are mostly atomic anyways, i dont think its a big deal to redefine them unless we are certain that this is actually a composed view of other atomic components.

src-docs/src/views/schema/schema_group.tsx

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

* Adds SchemaItem Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> * updates snapshot Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> * Adds changelog entry Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> * push fix changelog Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> * Address PR comments Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> (cherry picked from commit f3d42ef) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

* Adds SchemaItem Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> * updates snapshot Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> * Adds changelog entry Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> * push fix changelog Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> * Address PR comments Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> (cherry picked from commit f3d42ef) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

ashwin-pc added 2 commits August 17, 2023 01:36

Adds SchemaItem

b64c635

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

updates snapshot

0fa13eb

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

ashwin-pc requested review from seanneumann, AMoo-Miki, rednaksi91, KrooshalUX, BSFishy, bandinib-amzn, joshuarrrr, andreymyssak and SergeyMyssak as code owners August 17, 2023 01:39

github-actions bot added the first-time-contributor label Aug 17, 2023

ashwin-pc added 2 commits August 17, 2023 01:40

Adds changelog entry

2b52d0c

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

push fix changelog

df1838b

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

ashwin-pc force-pushed the schema-item branch from 650c88b to df1838b Compare August 17, 2023 01:42

Merge branch 'main' into schema-item

8cf2344

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

KrooshalUX approved these changes Aug 17, 2023

View reviewed changes

BSFishy reviewed Aug 17, 2023

View reviewed changes

joshuarrrr reviewed Aug 17, 2023

View reviewed changes

ashwin-pc added 2 commits August 18, 2023 00:19

Address PR comments

832a999

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

Merge branch 'main' into schema-item

b8f4d60

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

BSFishy approved these changes Aug 21, 2023

View reviewed changes

ashwin-pc merged commit f3d42ef into opensearch-project:main Aug 22, 2023

ashwin-pc added backport 1.x 1.3 labels Aug 22, 2023

opensearch-trigger-bot bot mentioned this pull request Aug 22, 2023

[Backport 1.x] Schema-item #980

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Schema-item #974

Schema-item #974

ashwin-pc commented Aug 17, 2023 •

edited

Loading

KrooshalUX left a comment

BSFishy left a comment

BSFishy Aug 17, 2023 •

edited

Loading

joshuarrrr Aug 17, 2023

ashwin-pc Aug 17, 2023

BSFishy Aug 17, 2023

joshuarrrr Aug 17, 2023

ashwin-pc Aug 17, 2023

joshuarrrr left a comment

joshuarrrr Aug 17, 2023

joshuarrrr Aug 17, 2023

joshuarrrr Aug 17, 2023

ashwin-pc Aug 17, 2023

joshuarrrr Aug 17, 2023

ashwin-pc Aug 17, 2023

	margin: $ouiSize / 2 0; // To align with buttonIcon
	margin: calc($ouiSize / 2) 0; // To align with buttonIcon

Schema-item #974

Schema-item #974

Conversation

ashwin-pc commented Aug 17, 2023 • edited Loading

Description

Issues Resolved

Check List

KrooshalUX left a comment

Choose a reason for hiding this comment

BSFishy left a comment

Choose a reason for hiding this comment

BSFishy Aug 17, 2023 • edited Loading

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

joshuarrrr left a 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

Choose a reason for hiding this comment

ashwin-pc commented Aug 17, 2023 •

edited

Loading

BSFishy Aug 17, 2023 •

edited

Loading