Semantic elements and layout #149

snowystinger · 2020-01-06T22:34:59Z

Closes https://jira.corp.adobe.com/browse/RSP-1467

Open questions:
Which components get a slot?

containers? (doesn't seem like enough)
semantic? (does that include divider? form elements?)
all? (seems like a lot)
answer, all components through useStyleProps

list of TODO's in the PR itself, just do a search for the string on the page

✅ Pull Request Checklist:

Included link to corresponding Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exist for this component).
Looked at the Accessibility Standard and/or talked to @mijordan about Accessibility for this feature.

📝 Test Instructions:

🧢 Your Team:

Added Image element, needs a rename, maybe Picture? Added slots to Divider/Label Added some Style stuff to Flex/Grid

…ently are removes circular dependency to use Slots in example story

adobe-bot · 2020-01-08T01:02:41Z

Build successful! View the storybook

packages/@react-types/shared/src/style.d.ts

adobe-bot · 2020-01-14T18:24:54Z

Build successful! View the storybook

LFDanLu

My initial gut feeling for your "Which components get a slot?" question is to say all components. I'm thinking that it would be cool for a end user to design an arbitrary grid-template-areas and be able to describe where any component would go just by passing in a slot prop. Dunno how well weirdly shaped components would fit in though so this might be unrealistic

packages/@react-spectrum/layout/src/Grid.tsx

packages/@react-spectrum/layout/src/Flex.tsx

packages/@react-spectrum/layout/src/Grid.tsx

packages/@react-spectrum/layout/src/move-to-somewhere/Image.tsx

packages/@react-spectrum/layout/src/move-to-somewhere/Text.tsx

packages/@react-types/divider/src/index.d.ts

packages/@react-types/shared/src/style.d.ts

devongovett · 2020-01-15T19:45:10Z

packages/@react-spectrum/divider/src/Divider.tsx

+    slot = 'divider',
    ...otherProps
  } = props;
+  let {[slot]: slotClassName} = useSlotProvider();


Can we do this within useStyleProps? It already returns a class name there, and all of our components already use it, so it might be easier to maintain in a single place.

devongovett · 2020-01-15T19:49:30Z

packages/@react-spectrum/layout/src/Flex.tsx

+    ...otherProps
+  } = props;
+  let {[slot]: slotClassName} = useSlotProvider();
+  // TODO: pull out into official handling


what does this mean?

referring to these functions justifyContent: ['justify-content', value => value] i wasn't sure where they should live, i think your other comment answered it though

the styleHandlers, wasn't sure where they should be, but i think your comment kinda answered it

devongovett · 2020-01-15T19:51:05Z

packages/@react-spectrum/layout/src/Flex.tsx

+import styles from './layout.css';
+import {useSlotProvider} from '@react-spectrum/utils';
+
+export interface FlexProps {


Should extend DOMProps and StyleProps. Also should be in @react-types

devongovett · 2020-01-15T19:56:44Z

packages/@react-spectrum/layout/src/move-to-somewhere/Keyboard.tsx

+import React, {ReactElement, ReactNode, RefObject} from 'react';
+import {useSlotProvider} from '@react-spectrum/utils';
+
+export interface KeyboardProps {


devongovett · 2020-01-15T19:56:50Z

packages/@react-spectrum/layout/src/move-to-somewhere/Keyboard.tsx

+  slot?: string
+}
+
+export const Keyboard = React.forwardRef((props: KeyboardProps, ref: RefObject<HTMLElement>) => {


Move to typography?

devongovett · 2020-01-15T19:57:05Z

packages/@react-spectrum/layout/src/move-to-somewhere/Text.tsx

+  slot?: string
+}
+
+export const Text = React.forwardRef((props: TextProps, ref: RefObject<HTMLElement>) => {


Move to typography. Did we discuss the naming of this element?

no, not yet, let's discuss! :)

devongovett · 2020-01-15T19:57:32Z

packages/@react-spectrum/layout/src/move-to-typography/Heading.tsx

+  slot?: string
+}
+
+export const Heading = React.forwardRef((props: HeadingProps, ref: RefObject<HTMLElement>) => {


devongovett · 2020-01-15T19:57:52Z

packages/@react-spectrum/layout/src/move-to-typography/Heading.tsx

+  let {[slot]: slotClassName} = useSlotProvider();
+
+  return (
+    <h1 {...filterDOMProps(otherProps)} ref={ref} className={classNames({}, slotClassName, className)}>


h1 should not be hard coded.

yep, was leaving alone so i didn't actually implement any of these sub elements
Image has caching stuff
None of them have spectrum- classes yet (mostly looking at typography with that statement)
types across all of them will probably need some alterations

I think i'd prefer to followup with that stuff, thoughts?

sounds fine to me

adobe-bot · 2020-01-16T19:07:01Z

Build successful! View the storybook

devongovett · 2020-01-16T22:24:05Z

packages/@react-types/shared/src/style.d.ts

  // transforms?
 }
+
+type globalVals = 'initial' | 'inherit' | 'unset';


Hmm, we haven't included these for any other style property. Do we need them? Is the property not being set equivalent?

I don't know, they are valid values, so figured they should be included and moved them to their own type so that they could be easily included on everything else should we decide we want them
also easy to delete if there is a strong feeling to not include them

devongovett · 2020-01-16T22:24:59Z

packages/@react-types/shared/src/style.d.ts

+type justifyContentType = 'center'| 'start'| 'end'| 'flex-start' | 'flex-end' | 'left' | 'right' | 'normal' | 'space-between' | 'space-around' | 'space-evenly' | 'stretch' | 'safe center' | 'unsafe center' | globalVals;
+type justifyItemsType = 'auto' | 'normal' | 'stretch' | 'center'| 'start'| 'end'| 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'left' | 'right' | 'baseline' | 'first baseline' | 'last baseline' | 'safe center' | 'unsafe center' | 'legacy right' | 'legacy left' | 'legacy center' | globalVals;
+type alignContentType = 'center'| 'start'| 'end'| 'flex-start' | 'flex-end' | 'normal' | 'baseline' | 'first baseline' | 'last baseline' | 'space-between' | 'space-around' | 'space-evenly' | 'stretch' | 'safe center' | 'unsafe center' | globalVals;
+type alignItemsType = 'normal'| 'stretch'| 'center'| 'start' | 'end' | 'flex-start' | 'flex-end' | 'baseline' | 'first baseline' | 'last baseline' | 'safe center' | 'unsafe center' | globalVals;


type names should be capitalized.

devongovett · 2020-01-16T22:26:04Z

packages/@react-types/shared/src/style.d.ts

+  justifyItems?: justifyItemsType,
+  alignContent?: alignContentType,
+  alignItems?: alignItemsType,
+  placeContent?: {


What is this property? Is it duplicating alignItems and justifyContent? Same for placeItems?

so placeContent is a valid css property that is a shorthand for /?
however, i couldn't figure out a good validation (typescript) pattern given that it could be any combination of the two or just a single value from align-content
so I broke it up into an object... got any ideas or just remove?
https://developer.mozilla.org/en-US/docs/Web/CSS/place-content

Ah ok. I think we can remove it since it's just a shorthand. Same below

devongovett · 2020-01-16T22:26:34Z

packages/@react-types/shared/src/style.d.ts

+    align: alignItemsType | 'auto'
+    justify?: justifyItemsType
+  },
+  rowGap?: DimensionValue // not well supported in Flex, but is well supported in Grid, also, should this really be dimension value??


Let's not include for flex then

packages/@react-spectrum/utils/src/styleProps.ts

devongovett · 2020-01-16T22:33:13Z

packages/@react-types/label/src/index.d.ts

  isRequired?: boolean,
-  necessityIndicator?: NecessityIndicator // default icon
+  necessityIndicator?: NecessityIndicator, // default icon
+  slot?: string


not needed (in style props)

devongovett · 2020-01-16T22:36:30Z

packages/@react-spectrum/layout/src/Grid.tsx

+    slots,
+    ...otherProps
+  } = props;
+  let {styleProps} = useStyleProps(otherProps, {...baseStyleProps, ...gridStyleProps});


Can baseStyleProps be included as part of gridStyleProps rather than here?

devongovett · 2020-01-16T22:36:36Z

packages/@react-spectrum/layout/src/Flex.tsx

+    children,
+    ...otherProps
+  } = props;
+  let {styleProps} = useStyleProps(otherProps, {...baseStyleProps, ...flexStyleProps});


Can baseStyleProps be included as part of flexStyleProps rather than here?

devongovett · 2020-01-16T22:37:31Z

packages/@react-spectrum/divider/src/Divider.tsx

+  let defaults = {
+    slot: 'divider'
+  };
+  props = {...defaults, ...props};


Maybe we could pass the defaults as a second param to useStyleProps and merge them in there instead of doing this in every component?

useStyleProps(otherProps, {slot: 'divider'})

would you want it as the third?
currently the signature is useStyleProps(props: StyleProps, handlers: StyleHandlers = baseStyleProps)

also, do we want to set defaults for any of the styleProps object? 'style' or 'className' etc?

Hmm, an alternative:

useStyleProps({slot: 'divider', ...otherProps})

devongovett · 2020-01-16T22:38:16Z

packages/@react-spectrum/layout/src/layout.css

+TODO: find home for this, rsp? spectrum-css?
+*/
+
+.flex {


sure, it's only one property though, and all the other style props on these components are inline already...

adobe-bot · 2020-01-17T22:06:15Z

Build successful! View the storybook

packages/@react-types/divider/src/index.d.ts

devongovett · 2020-01-18T01:16:03Z

packages/@react-types/shared/src/style.d.ts

  flexShrink?: number,
  justifySelf?: 'auto' | 'normal' | 'start' | 'end' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'center' | 'left' | 'right' | 'stretch', // ...
  alignSelf?: 'auto' | 'normal' | 'start' | 'end' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'center' | 'stretch', // ...
+  placeSelf?: any, // don't know how to type


Remove? I think this is just a shorthand for a couple other properties represented here so probably no need for this

devongovett · 2020-01-18T01:16:20Z

packages/@react-types/shared/src/style.d.ts

+  justifyItems?: justifyItemsType,
+  alignContent?: alignContentType,
+  alignItems?: alignItemsType,
+  placeContent?: {


Ah ok. I think we can remove it since it's just a shorthand. Same below

devongovett · 2020-01-18T01:19:24Z

packages/@react-spectrum/layout/src/Grid.tsx

+    slots,
+    ...otherProps
+  } = props;
+  let {styleProps} = useStyleProps(otherProps, {...gridStyleProps});


No need to clone gridStyleProps right?

devongovett · 2020-01-18T01:19:34Z

packages/@react-spectrum/layout/src/Flex.tsx

+    children,
+    ...otherProps
+  } = props;
+  let {styleProps} = useStyleProps(otherProps, {...flexStyleProps});


adobe-bot · 2020-01-21T19:30:26Z

Build successful! View the storybook

adobe-bot · 2020-01-21T19:33:04Z

Build successful! View the storybook

adobe-bot · 2020-01-22T18:36:18Z

Build successful! View the storybook

adobe-bot · 2020-01-22T23:06:04Z

Build successful! View the storybook

devongovett

Looks good. Please fix the 2 items above and then merge.

devongovett · 2020-01-23T00:21:55Z

packages/@react-spectrum/utils/src/styleProps.ts

+  alignItems: ['align-items', passthroughStyle],
+  alignContent: ['align-content', passthroughStyle],
+  placeItems: ['place-items', placementStyle],
+  placeContent: ['place-content', placementStyle],


Still think we should remove these since they're just shorthands... Looks like they're already gone in the types.

devongovett · 2020-01-23T00:22:37Z

packages/@react-spectrum/utils/src/styleProps.ts

+  placeItems: ['place-items', placementStyle],
+  placeContent: ['place-content', placementStyle],
+  rowGap: ['row-gap', dimensionValue],
+  columnGap: ['row-gap', dimensionValue],


I think these should only be in grid since they are not widely supported for flex layout. Seems done in the types already too.

adobe-bot · 2020-01-23T18:21:43Z

Build successful! View the storybook

ktabors

Seems pretty straightforward and cool

ktabors · 2020-01-22T23:12:03Z

packages/@react-spectrum/image/package.json

@@ -0,0 +1,40 @@
+{
+  "name": "@react-spectrum/image",
+  "version": "3.0.0-rc.1",


3.0.0-alpha.1

ktabors · 2020-01-22T23:29:27Z

packages/@react-spectrum/image/package.json

+  "dependencies": {
+    "@babel/runtime": "^7.6.2",
+    "@react-aria/utils": "^3.0.0-rc.1",
+    "@react-spectrum/provider": "^3.0.0-rc.1",


peer dependency

ktabors · 2020-01-22T23:54:32Z

packages/@react-spectrum/image/src/Image.tsx

+  let {styleProps} = useStyleProps({slot: 'image', ...otherProps});
+  let domRef = useDOMRef(ref);
+
+  if (decorative) {


!decorative? ... I know demo component

ktabors · 2020-01-22T23:56:05Z

packages/@react-spectrum/image/src/Image.tsx

+    <div
+      {...filterDOMProps(props)}
+      {...styleProps}
+      style={{overflow: 'hidden'}}


if not a demo component I'd want style done differently

yep, demo, i'm not doing anything with image right now

packages/@react-spectrum/layout/package.json

ktabors · 2020-01-23T18:18:01Z

packages/@react-spectrum/layout/test/Grid.test.js

+import React from 'react';
+
+// TODO: testing :)
+describe('Grid', function () {


What would a grid test look like since it's basically a container for CSS and children?

left over from plop, i don't know that it needs one right now, when i think of one, i'll write it

ktabors · 2020-01-23T18:18:54Z

packages/@react-spectrum/typography/package.json

+  "dependencies": {
+    "@babel/runtime": "^7.6.2",
+    "@react-aria/utils": "^3.0.0-rc.1",
+    "@react-spectrum/provider": "^3.0.0-rc.1",


peer dependency

ktabors · 2020-01-23T18:22:37Z

packages/@react-spectrum/utils/src/Slots.ts

@@ -0,0 +1,7 @@
+import React, {useContext} from 'react';


why is there here instead of @react-spectrum/slots

seemed heavy to include yet another package, @devongovett you have any preferences?

Yeah I think utils is fine for this

ktabors · 2020-01-23T18:40:49Z

packages/@react-spectrum/view/src/Content.tsx

+  children: ReactElement | string | ReactElement[]
+}
+
+export const Content = React.forwardRef((props: ContentProps, ref: RefObject<HTMLElement>) => {


Naming is hard, is there a better name for this? Content feels too generic, like I get what a header or footer is, but content could be anything. It feels more like body, but that is taken. copy, gist, text, subject, message????

content is meant to be extremely generic for this, it's to represent the
tag, but Section doesn't mean much to people
So this is like, the body content of a dialog, the body content of a card etc

ktabors · 2020-01-23T18:42:08Z

packages/@react-spectrum/view/src/Footer.tsx

+  let {styleProps} = useStyleProps({slot: 'footer', ...otherProps});
+
+  return (
+    <footer {...filterDOMProps(otherProps)} {...styleProps} ref={ref}>


the slot, name, and html tag are the only difference at this time between any of these. Is there a more interesting way to generalize and compose these?

eventually they will probably have some associated spectrum styles for typography
the slot is the most important part, these all have defaults for grid-area so that people don't need to think about slots for the most part

LFDanLu

lgtm

adobe-bot · 2020-01-23T21:46:08Z

Build successful! View the storybook

Co-authored-by: Yihui Liao <44729383+yihuiliao@users.noreply.github.com> Co-authored-by: Rob Snow <rsnow@adobe.com>

snowystinger added 2 commits January 2, 2020 20:56

Separating menu items from semantic elements and layout

91df7f1

Card example story looks good

976fe9b

Added Image element, needs a rename, maybe Picture? Added slots to Divider/Label Added some Style stuff to Flex/Grid

snowystinger requested review from dannify and devongovett as code owners January 6, 2020 22:34

move some things around because they don't make sense where they curr…

4a5c0fb

…ently are removes circular dependency to use Slots in example story

snowystinger changed the title ~~Semantic elements and layout~~ WIP: Semantic elements and layout Jan 6, 2020

update so build works

3eaebc7

pst67662 reviewed Jan 10, 2020

View reviewed changes

packages/@react-types/shared/src/style.d.ts Outdated Show resolved Hide resolved

Merge branch 'master' into semantic-elements-and-layout

e934c58

LFDanLu requested changes Jan 15, 2020

View reviewed changes

devongovett reviewed Jan 15, 2020

View reviewed changes

snowystinger added 3 commits January 15, 2020 17:00

break point for end of the day

6188b34

Merge branch 'master' into semantic-elements-and-layout

3a4e162

push a working build

2cdd688

devongovett reviewed Jan 16, 2020

View reviewed changes

more fixes

8bb0fc8

devongovett reviewed Jan 18, 2020

View reviewed changes

snowystinger added 2 commits January 21, 2020 11:25

couple last cleanups

a87c834

Merge branch 'master' into semantic-elements-and-layout

3ad4bbf

snowystinger changed the title ~~WIP: Semantic elements and layout~~ Semantic elements and layout Jan 21, 2020

snowystinger added 3 commits January 22, 2020 10:17

change where slot default is defined

546b7d0

Merge branch 'master' into semantic-elements-and-layout

2877c5c

overzealous deletion

491816f

Fix version numbers

5cff69e

devongovett previously approved these changes Jan 23, 2020

View reviewed changes

remove placement style shortcuts

eec8704

snowystinger dismissed devongovett’s stale review via eec8704 January 23, 2020 18:16

devongovett approved these changes Jan 23, 2020

View reviewed changes

ktabors reviewed Jan 23, 2020

View reviewed changes

LFDanLu approved these changes Jan 23, 2020

View reviewed changes

Merge branch 'master' into semantic-elements-and-layout

041b140

snowystinger merged commit 2ee8329 into master Jan 23, 2020

snowystinger deleted the semantic-elements-and-layout branch January 23, 2020 21:46

devongovett pushed a commit that referenced this pull request Jul 25, 2024

should be able to apply width to tag group + truncate behavior (#149)

7c39cdf

Co-authored-by: Yihui Liao <44729383+yihuiliao@users.noreply.github.com> Co-authored-by: Rob Snow <rsnow@adobe.com>

Semantic elements and layout #149

Semantic elements and layout #149

Uh oh!

Conversation

snowystinger commented Jan 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Team:

Uh oh!

adobe-bot commented Jan 8, 2020

Uh oh!

Uh oh!

adobe-bot commented Jan 14, 2020

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Jan 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowystinger commented Jan 6, 2020 •

edited

Loading

devongovett Jan 18, 2020 •

edited

Loading