feat(@clayui/layout): create low-level layout utilities #3204

bryceosterhaus · 2020-05-07T20:09:02Z

Here is my initial idea for the low-level components of row, col, and container.

cc @pat270 can you checkout the demos on storybook and verify these make sense?

coveralls · 2020-05-07T20:14:20Z

Pull Request Test Coverage Report for Build 110730824

59 of 67 (88.06%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 78.614%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/clay-layout/src/ContainerFluid.tsx	4	5	80.0%
packages/clay-layout/src/Content.tsx	9	12	75.0%
packages/clay-layout/src/Sheet.tsx	11	15	73.33%

Totals
Change from base Build 110265687:	0.04%
Covered Lines:	2367
Relevant Lines:	2805

💛 - Coveralls

jbalsas · 2020-05-08T07:45:24Z

packages/clay-layout/src/Col.tsx

+	sm,
+	xs,
+	...otherProps
+}) => {


Nice!

We didn't add align and order to our JSP implementation... might be worth doing based on this! 🤔

My goal was to mimic the docs examples as much as possible without having the user to have to add custom classes.

We even don't recommend to use order since it causes accessibility problems of tab perception, I would not add it to avoid encouraging its use

jbalsas · 2020-05-08T07:47:52Z

packages/clay-layout/src/Container.tsx

+	/**
+	 * Adds `.container-fluid-${size}` class to set max width on container.
+	 */
+	fluidSize?: 'sm' | 'md' | 'lg' | 'xl';


We just called this size in our JSP tag since it seems to be the basic function of the Container, so why the prefix?

We also default to xl since it's by far the main usage we saw across the board... might want to consider it here too

I used the prefix since I didn't want people to think size could correlate to container-${size}. I thin by making it explicitly named fluid its apparent that its for the container-fluid class

I think the correct name properties should be fluidMaxSize since we only use classes container-fluid-max-*breakpoint* to create all Lexicon grids they need. On the other hand, I would recommend forgetting all rest of ways to create container size limit because there are a lot which probably a final customer could use but we do not need

So... now that we have container-fluid... maybe this could be:

fluid: boolean | ['sm', 'md', 'lg', 'xl']?

jbalsas · 2020-05-08T07:50:30Z

packages/clay-layout/src/Container.tsx

+	 * Adds `.container-fluid` class to create a fluid container that
+	 * doesn't expand beyond a set width
+	 */
+	fluid?: boolean;


I lost track of where, but we went through a back and forth over To Fluid or Not To Fluid and concluded that we didn't want To Fluid 😂

So, by default, we'd have <Container>...</Container> output:

<div class="container-fluid container-fluid-xl"> ... </div>

This is somewhat opinionated... but so is our design language and as long as this can be overridden we should be fine. Not sure we have a valid case for just container, at the very least is not widespread enough to grant a first-level API.

@eduardoallegrini, @marcoscv-work, mind weighing in on this?

Hmm, I don't think I was privy to that convo.

IMO, I think its really odd to default to container-fluid for a component called Container, especially when the class container is also a thing.

I think it'd be fine to keep container-fluid as a default for the a taglib in portal, but for the Clay library I don't think it would make sense to default to fluid for the name Container

Initially, my concern about the using of container when we already have a class called container was the same @bryceosterhaus already has wrote, after some days taking a look to what lexicon want and what we finally use, the uses cases are only a few, majority of them are the classes container-fluid container-fluid-max-xl and we don't use the standard bootstrap container.
So for us and right now the default container is the class combination: container-fluid container-fluid-max-xl. It still sounds bad to me but I have to admit our container is a no very semantic combination of classes, maybe it's something we can study and try to improve in a future more semantic CSS.

IMHO, the <Container> output should be our standard and more used container

jbalsas · 2020-05-08T07:51:24Z

packages/clay-layout/src/Container.tsx

+	 * between application controls and the form. This class only modifies
+	 * the padding on the container.
+	 */
+	formSize?: 'sm' | 'md' | 'lg' | 'xl';


This is actually an interesting take... we treated this as className but it might make sense as an API...

@eduardoallegrini, thoughts?

I went through our documentation and tried to expose API that we explicitly talk about in the css or in our layouts definition. This is why I added it.

I think it can be confusing with form sizes, if we speak about 'padding or margin' this probably should be 'spacers'. We have 8 unit sizes defined:

clay/packages/clay-css/src/scss/atlas/variables/_globals.scss

Lines 182 to 211 in 0273c22

$spacers: map-deep-merge(

(

0: 0,

1: (

$spacer * 0.25,

),

2: (

$spacer * 0.5,

),

3: (

$spacer * 1,

),

4: (

$spacer * 1.5,

),

5: (

$spacer * 3,

),

6: (

$spacer * 4.5,

),

7: (

$spacer * 6,

),

8: (

$spacer * 7.5,

),

),

$spacers

);

It generates all famous helpers like: m-1, pt-5, and our won utilities helpers: c-m-5, c-pt-5, etc.

Info about forms size:

For forms we have two groups sizes, and it can be consider a container: the default one form-group and form-group form-group-sm

for individual forms (form-controls) we have three sizes:

I would remove this one, just on the basis that it's easier to add them later than to remove them.

We can implement for now using className, and can bring it as an API once the pattern is more clear?

jbalsas · 2020-05-08T07:51:41Z

packages/clay-layout/src/Container.tsx

+	 * controls and view pages (e.g., Card View, Table View, or List View).
+	 * This class only modifies the padding on the container.
+	 */
+	view?: boolean;


Same as with .formSize...

@eduardoallegrini, thoughts?

I'm unsure as to the importance of these "proprietary" classes if we're going with the approach of separating the library from Portal. If I were making an app using Clay, I wouldn't really bother too much with props that have these very specific use cases.

Perhaps the question to ask is: "How often will the user use this component with pre-existing knowledge of which props they'll need?"
If it were me, I'd use <ClayContainer flexOrEquivalent /> and then build out the rest, and then when it came to styling I'd probably not go back and check every component to see if I can configure the layout using props but instead just work in CSS.

I were making an app using Clay, I wouldn't really bother too much with props that have these very specific use cases [...]

[...] I'd probably not go back and check every component to see if I can configure the layout using props but instead just work in CSS [...]

That begs the question... why would you bring in a library dependency such as Clay to substitute <div> by <ClayContainer> and keep the rest all the same?

If you won't bother checking the props and defaults of the library of your choosing, why'd you choose it in the first place?

We've discussed at length our vetting process for picking dependencies that are actually worth their value. If we were to have that conversation around this library, maybe @wincent, or yourself would argue that we don't need Clay since we already have flexOrEquivalent.

That might mean, you wouldn't be a user of Clay in a normal situation, so we shouldn't model it around this preference :)

The fact that the primary goal of Clay is to serve DXP makes my concern kinda obsolete. And that's fine, I was just looking out for the little man here 😄, all of us using Clay while building DXP will look into all the props and use them accordingly. So having the classes that are omnipresent throughout DXP in this component as props is a time saver for us.

[...] primary goal of Clay is to serve DXP [...]

I didn't say this!!

Primary goal of Clay is to make building Lexicon UIs easy, consistent, robust and safe.

DXP is the main consumer right now, so we shouldn't fall into the common trap of serving imaginary users and failing to deliver to those real teams right now. We've been down that path before. It ain't pretty 😉

That being said, we also aim for other departments and teams to jump on board as Design practices converge. AC (Faro), IS, DXP Cloud... they will all benefit in the long run if we do a good job here. That's why looking out for those "general" users is still valid and important, just not at the expense of DXP.

Yea, I was worried my wording was too strong. I get what you're saying 👍 . I was focusing on the imaginary users more than on DXP. That's what makes Lexicon great, as long as we make components based on their design it should be great for both DXP and the other projects Liferay has, right?

That's what makes Lexicon great, as long as we make components based on their design it should be great for both DXP and the other projects Liferay has, right?

That's the dream 😂

Similar to formSize... I know this is being used, si this has indeed real backing, but I wouldn't commit to it too early.

jbalsas · 2020-05-08T07:53:27Z

packages/clay-layout/src/Row.tsx

+	gutters = true,
+	justify,
+	...otherProps
+}) => {


We didn't add any of these to <clay:col> since we couldn't find real usages, so it might be premature API, but LGTM

I think its hard for devs to remember the util classes for alignment, my thought was by adding the align and justify props, it would be more accessible to devs via ts autocomplete.

jbalsas · 2020-05-08T07:54:46Z

packages/clay-layout/src/__tests__/__snapshots__/index.tsx.snap

+exports[`ClayLayout renders 1`] = `
+<div>
+  <div
+    class="container"


As I said above, if we want to somewhat match expectations, we might want to be a tad bit opinionated here and have this be container-fluid container-fluid-xl which is by far our most beloved container 😂

I think I would push back on that for the library level. Unless you can think of a good naming convention for the component to indicate it will include those. For a user to use <Container> without props and then get those classes seems odd to me. On the portal side of things, we can default to that since it makes sense for that to be portal's default, but not sure it makes sense to default at the library level.

I would also potentially push back and add that if we expect the default behavior of container to be container-fluid container-fluid-xl we might consider making that at the css level.

I agree with @bryceosterhaus here. Default values can be provided in the implementation on Portal, thus preserving abstraction for the end-user while still providing users of Clay as a library the flexibility.

Hey @bryceosterhaus, @kresimir-coko, I'll try to go at some more length in the general comments, but:

Once again, I think we're failing to see that right now, 100% (unless Faro moved to Clay) of our users are DXP. Also, 90% of our usage when implementing lexicon is container-fluid. This means that by not providing this we're refusing to provide real value to real users of our library based on some not-so-real user/library expectation. Since 90% of the times someone in Liferay looking to build a UI had to use container-fluid, it is our goal to make this easier for them.

Please, go through:

https://github.com/jbalsas/liferay-portal/pull/2160/commits/e72f30c231e196b72dcf15bbff7d94ddda6b2655

https://github.com/eduardoallegrini/liferay-portal/pull/71/commits

edalgrin/liferay-portal@4c9f07b

The main goal of Clay is to simplify implementing Lexicon UIs. The main implementor of Lexicon UIs is DXP. Based on the above changes, I'd say those are better defaults for implementing Lexicon UIs. Thus, those defaults belong in Clay, not in DXP. Otherwise, we're not making it any easier. There are literally thousands of occurrences of container-fluid and around tens of container. It doesn't make any sense that we'd favour the latter here.

If you want to be real purist here, let's maybe create FluidContainer that composes Container and provides the right semantics and ergonomics to our devs. We can do the same in the Portal side and mostly simply use <clay:fluid-container> and <ClayLayout.FluidContainer>.

@wincent, @marcoscv-work, thoughts?

This means that by not providing this we're refusing to provide real value to real users of our library based on some not-so-real user/library expectation.

Yeah this totally makes sense, my thought is based more css vs react component discrepancies. My main concern is that we should have consist naming in our components that stem from @clayui/css. By defaulting ClayLayout.Container to be fluid, seemed to be at odds with @clayui/css. Also, if Lexicon is recommending the styles of .container-fluid to be used 90% of the time, and .container just 10% of the time, maybe we should just have one class and not use the non-fluid styles.

If you want to be real purist here, let's maybe create FluidContainer that composes Container and provides the right semantics and ergonomics to our devs.

I'm not sure I would label myself as a purist 😂, I just want to make sure we are consistent from @clayui/css to our react components. I like this idea for naming though. Maybe even going with ClayLayout.ContainerFluid for more consistency. I think this would be inline with both @clayui/css and the need in DXP.

@kresimir-coko
I would really dislike going into DXP code a couple years from now and seeing everywhere

What would be the issue with this? I don't think it'lI be an issue really and I think it will actually be better for readability in the future and knowing exactly what the containers purpose is. I also don't mind it because it'll be easy to find/replace for any future refactors

@wincent, @marcoscv-work, thoughts?

We could create FluidContainer and Container to fit semantic but finally, we wouldn't use Container for anything, so I'm closer to fix the semantic in future versions of CSS than creating components or variations we don't really need

Actually, when we weigh in our autofit rows and cols this might play a part since those do more or less need an inert container... so Container (without the "container" class actually... 😂) could help bridge the gave between the different grids.

Or maybe something else, so we'd have

Layout (Stack) -> Container/FluidContainer -> Row/Col
|-> Row autofit -> Col autofit

Adding one additional layer feels a bit over-engineered... so maybe we could simplify with:

<Container> -> <div class="container">...</div> <Container autofit> -> <div>...</div> <FluidContainer> -> <div class="container-fluid container-fluid-max-xl">...</div>

Container 'type' autofit sounds good to me, if it makes clear this is not the BS container is enough.

It is out of time but I was thinking about something more semantic which wraps everything, something like:

Using

<Grid> <Grid container> <Grid row> <Grid col> <Grid autofit-row> <Grid autofit-col>

jbalsas

Hey @bryceosterhaus! This looks pretty solid to me, just left some comments regarding aligning some APIs and outputs with what we found based on real usage.

Looks like a solid start to me! 👍

wincent · 2020-05-08T08:11:26Z

@jbalsas already beat me to it on the subject of cross-implementation consistency (which I think is pretty desirable), but I was going to provide a few links that could be useful for comparing against the taglib API:

(Edited to provide current HEAD of master links.)

bryceosterhaus · 2020-05-08T18:04:41Z

aligning some APIs and outputs with what we found based on real usage.

I think we should probably clarify expectations on this a bit. Are we thinking the taglibs and the components should be 1-for-1 API match? IMO I think they should be similar but not exact. If we make them exact, I think it forces clay to export odd defaults to the API that would be unexpected for a user. I made a comment about this on the container component with the container-fluid class above.

On the portal side, I think it makes sense to default values though..

// via jsx
<Clay.Container />

// renders
<div class="container" />




// jsp
<clay:container />

// clay:container internal use
<Clay.Container fluid fluidSize="xl" />

// renders
<div class="container-fluid container-fluid-max-xl">

Otherwise, if we are pushing that the default Container of clay to be container-fluid container-fluid-max-xl, then maybe that should be the default on the css side for .container

Does my line of thinking make sense here or am I wildly off?

bryceosterhaus · 2020-05-08T20:11:16Z

added an additional commit. Went ahead and added the row-cols-${size}-${value} API to row. This will make it easy to configure a full grid. I also updated some demos and added containerElement prop which allows users to provide their own component or a different html element.

kresimir-coko · 2020-05-11T07:04:17Z

packages/clay-layout/src/Row.tsx

+	/**
+	 * Vertical positioning of row contents
+	 */
+	align?: 'start' | 'center' | 'end';


align-items has more than these 3 options. MDN on align-items

Yeah I am following bootstraps docs on their alignment classes. https://getbootstrap.com/docs/4.0/layout/grid/#vertical-alignment

I think we can start with these 3 for now and then evaluate later if people need more.

kresimir-coko · 2020-05-11T07:05:05Z

packages/clay-layout/src/Row.tsx

+	/**
+	 * Horizontal positioning of row contents
+	 */
+	justify?: 'start' | 'center' | 'end' | 'around' | 'between';


Same as align-items, has more options than the ones listed here.

Same as above comment, I am trying to keep it minimal and follow bootstraps grid docs for what should be available.

kresimir-coko · 2020-05-11T07:13:29Z

packages/clay-layout/stories/index.tsx

+import ClayLayout from '../src';
+
+const exampleStyles = {
+	backgroundColor: 'rgba(86,61,124,.15)',


Not sure if it's too much, maybe you can provide some insight @pat270, but what do you think about using some brand colors here? Adding some branding to storybook might make it look more "ours" if you know what I mean.

This sounds good to me. Maybe #E7E7ED or #cdced9?

kresimir-coko · 2020-05-11T07:24:12Z

packages/demos/stories/ListPage.tsx

@@ -296,7 +297,7 @@ export default () => {
 				)}
 			</ClayManagementToolbar>

-			<div className="container" style={{paddingTop: 8}}>
+			<ClayLayout.Container style={{paddingTop: 8}}>


In the words of my toxic teacher from elementary school: "8 what? Potatoes?".
Shouldn't we explicitly say pixels here?

I don't think its a huge issue for us to specify since in React we know that any style that is applied via integer becomes a px. I would agree with you in the case of a string, then we would need to specify.

https://reactjs.org/docs/dom-elements.html#style

React will automatically append a “px” suffix to certain numeric inline style properties. If you want to use units other than “px”, specify the value as a string with the desired unit.

jbalsas · 2020-05-11T07:36:05Z

Hey @bryceosterhaus!

Are we thinking the taglibs and the components should be 1-for-1 API match? IMO I think they should be similar but not exact.

They can't really be 1-for-1 since Java-JS doesn't translate all that well. That being said, the closer the better.

If we make them exact, I think it forces clay to export odd defaults to the API that would be unexpected for a user.

This is a different topic. I also answered in the container-fluid discussion. To summarize, I believe Clay should provide the defaults that make sense to achieve its main goal. Clay's goal is making it easier to build Lexicon UIs. Based on real experience building Lexicon UIs, those defaults are not in this PR.

// jsp
<clay:container />

// clay:container internal use
<Clay.Container fluid fluidSize="xl" />

One source of confusion I see here is that you seem to think JSP implementations actually use React components... which will be true and not true at the same time. We currently don't have SSR, so JSPs do not defer on React components to render. They do to hydrate for interactive components, though.

Otherwise, if we are pushing that the default Container of clay to be container-fluid container-fluid-max-xl, then maybe that should be the default on the css side for .container

Does my line of thinking make sense here or am I wildly off?

I think there's a mixup with 2 concerns.

What are the right defaults for Clay as a Library? -> Those that simplify building Lexicon UIs. Forcing a developer to always write <Clay.Container fluid fluidSize="xl"> means the defaults we provide are not valuable since they're of no use 90% of the time. What good are defaults that only work 10% of the time?
Defaulting <Container> to .container-fluid seems odd to you, and on that I can agree. Maybe a middle ground would be to create a "highish" lower level component like FluidContainer that composes Container and provides the right semantics and ergonomics to our devs.

bryceosterhaus · 2020-05-11T19:51:48Z

@jbalsas I commented on the container-fluid comment as well, but I'll respond to

create a "highish" lower level component like FluidContainer that composes Container and provides the right semantics and ergonomics to our devs.

Yeah I think this is the best means forward for this since it keeps consistency with @clayui/css and also provides a helpful OOTB solution for dxp.

On similar note, I created an issue to further decide how to avoid these reoccurring discussions around clay + dxp. I think the confusion comes up because we have so many different layers(dxp, taglibs, react components, css) that are intertwined and don't necessarily have clear definitions to guide them. #3214 Please use this issue for continued high-level comments.

bryceosterhaus · 2020-05-11T21:35:05Z

updated with ContainerFluid component

jbalsas · 2020-05-12T14:09:22Z

packages/clay-layout/src/ContainerFluid.tsx

+	size?: React.ComponentProps<typeof Container>['fluidSize'];
+}
+
+const ClayContainerFluid: React.FunctionComponent<IProps> = ({


LGTM!

nit: Should it FluidContainer instead of ContainerFluid?

Since its .container-fluid, it made more sense to me in keeping it consistent with that. I also prefer NounDescriber(if that makes sense)

I think it's weird... didn't we do the opposite in the other components in this PR?

I'm thinking now at ContentSection, ContentRow and ContentCol... it might just be my lack of familiarity with the grammar, but those feel like DescriberNoun. Basically i can tell because describer is common and noun is the thing that changes at the end.

It's true that we don't have many such cases in Clay, but others I found that also seem to contradict this are ItemField, ItemText and ItemTitle...

Ah, yeah, never mind, I think I see it now... 😂 👍

Or maybe not! 😂 @wincent, @pat270, some native english speakers here, please? I'm fine either way 👍

Just to state my confusion.

ContentSection, ContentRow and ContentTitle... one would assume as follows that we might have a ContentContainer.

ItemField, ItemText, ItemTitle... similarly, I'd think an ItemContainer seems logical...

From the above, FluidContainer seems to follow the sequence... but I was never good at this 😂

Native English speaker here... English laughs at your pathetic request to have any kind of consistent rules.

(I know the gif is "French", but it accurately summarizes the English attitude to consistency.)

The general grammatical pattern in English is to put modifiers (adjectives) before nouns (eg. "red car"). Things get complicated because English is quite happy to use nouns as modifers too (ie. noun like "money" being used like an adjective in the term "money bag") — but still, the modifier goes first. Unlike in Spanish, which is pretty much the opposite (eg. "coche rojo").

But programming is another layer above English, so there are other concerns beyond mere grammar. For example, we like to group related things together using a prefix. That's why ItemField, ItemText, ItemTitle start with a common prefix: the "thing" here is "Field", "Text", "Title", but the common relationship is that they are all the "thing" of an "Item", so we extract the common bit out and stick it at the front. And even more obviously, everything in Clay starts with "Clay", not because "Clay" is a modifier or an adjective or a noun or anything — it's the common root that it all relates to.

With respect to ContainerFluid vs FluidContainer, I have no idea. The "thing" it is is a container, and "Fluid" is the modifier that describes the container, so I'd expect it to be FluidContainer. But as @bryceosterhaus said, he was being consistent with .container-fluid.

yeah the more I think about it I really have no idea why somethings sounds "more right" than the other way around. So I think I'll just rest my decision based on the class name itself.

jbalsas · 2020-05-12T14:12:30Z

@bryceosterhaus, thanks for the update, that makes sense to me!

At the risk of dragging this over for an additional day, do you mind weighing over at https://docs.google.com/document/d/13K-jishFtNLOoWCsWffjsCc6xsfaiH9_-ofXwoGdx3Y? (and anyone else interested, of course... looking at you, @kresimir-coko, @matuzalemsteles, @pat270...).

I think we might be close to getting something more or less coherent that covers our most used cases that involve what we have here plus the autofit-row + autofit-col cases... plus some additional higher levels like <Sheet> and <Card> that we might or might not need to add high-level components for...

Looking for everyone to have a bit of a bigger picture to avoid too much back and forth in the implementation during the next few days, but certainly looking to wrap this up no later than this week (if possible) and continue iterating.

pat270 · 2020-05-12T16:07:38Z

@bryceosterhaus The only thing that caught my eye on the storybook demos was:

<ClayLayout.Row>
	<ClayDemoColumn md={8} size={12}>
		{'.col-12 .col-md-8'}
	</ClayDemoColumn>
</ClayLayout.Row>

having the size attribute output .col-# seems a little odd. Bootstrap groups all the .col-# classes under XS, see https://getbootstrap.com/docs/4.4/layout/grid/#grid-options.

bryceosterhaus · 2020-05-13T20:59:03Z

Updated with a few additional ideas. <ClayLayout.Content* is a component that uses "autofit-*" classes. See 6244ab8#diff-58014a94aedc67ab5431dd06b8a5bf63 for what it would look like in action.

jbalsas · 2020-05-14T07:26:08Z

Updated with a few additional ideas. <ClayLayout.Content* is a component that uses "autofit-*" classes. See 6244ab8#diff-58014a94aedc67ab5431dd06b8a5bf63 for what it would look like in action.

Interesting @bryceosterhaus! I was playing around with the idea of an autofit attribute yesterday at https://github.com/jbalsas/liferay-portal/pull/2166.

I think this is more aligned with what @marcoscv-work envisioned... as we discussed, the problem here is that now you need to know 2 different set of APIs... but you did have to know about autofit anyways, so it doesn't look so different. I can be convinced and follow this to match our jsp tags...

Also, if the desire is to move away from lowish-level container-row-col to more content-aware components this seems like a good move...

@marcoscv-work, @eduardoallegrini, @wincent... thoughts?

wincent · 2020-05-14T09:20:44Z

@marcoscv-work, @eduardoallegrini, @wincent... thoughts?

I'd much rather hear from the first two people you mention, @jbalsas; I don't want to contribute any noise to an already very dense thread.

marcoscv-work · 2020-05-14T13:15:54Z

Updated with a few additional ideas. <ClayLayout.Content* is a component that uses "autofit-*" classes. See 6244ab8#diff-58014a94aedc67ab5431dd06b8a5bf63 for what it would look like in action.

Interesting @bryceosterhaus! I was playing around with the idea of an autofit attribute yesterday at jbalsas/liferay-portal#2166.

I think this is more aligned with what @marcoscv-work envisioned... as we discussed, the problem here is that now you need to know 2 different set of APIs... but you did have to know about autofit anyways, so it doesn't look so different. I can be convinced and follow this to match our jsp tags...

Also, if the desire is to move away from lowish-level container-row-col to more content-aware components this seems like a good move...

@marcoscv-work, @eduardoallegrini, @wincent... thoughts?

That's closer to what I tried to explain, it looks more semantic and I really like it.
If we agree we can use the pattern in clay and Liferay portal :-D

jbalsas · 2020-05-16T16:36:44Z

packages/clay-layout/src/Content.tsx

+	 * Provides the benefit of aligning content via flexbox without losing the behavior
+	 * of floated elements at the expense of extra markup.
+	 */
+	float?:


Not sure what I'll name this in Java since float is a reserved keyword 😂

jbalsas · 2020-05-18T14:24:01Z

Hey @bryceosterhaus, this looks almost good to go to me...

I just added a question about the padded and noGutters relation and a couple more comments here and there about APIs we might not want to commit to without more data.

My suggestion would be to remove those props for now, and go through LPS-111257 Analyze Layout Strategies used in React Components in DXP - Report based on an initial release (or local) of this. Keep in mind that I think that's missing autofit and sheet usage, so those should be added too.

With that, we should have first-hand visibility on these APIs and their ergonomics. We can create some LPSs to send updated Apps to their teams using the new components.

What do you think?

bryceosterhaus · 2020-05-18T20:08:39Z

My suggestion would be to remove those props for now,

Are we sure we want to remove them? *-padded and *-padded-no-* are used at least 20 times in DXP from my count. Also, it seems from the conversation that there isn't really any coding needed to link the two classes together since they behave independently.

bryceosterhaus · 2020-05-18T20:09:54Z

We can create some LPSs to send updated Apps to their teams using the new components.

Yeah I am up for this. I think it'd be wise to get an initial release of @clayui/layout merged into portal first and then we can go ahead and make these updates and LPSs.

jbalsas · 2020-05-18T20:11:47Z

Are we sure we want to remove them? *-padded and -padded-no- are used at least 20 times in DXP from my count. Also, it seems from the conversation that there isn't really any coding needed to link the two classes together since they behave independently.

No, I meant the view, formSize and order. The padded ones make sense to me and are pretty straightforward.

jbalsas · 2020-05-18T20:12:17Z

Yeah I am up for this. I think it'd be wise to get an initial release of @clayui/layout merged into portal first and then we can go ahead and make these updates and LPSs.

SGTM

bryceosterhaus · 2020-05-18T20:22:28Z

No, I meant the view, formSize and order. The padded ones make sense to me and are pretty straightforward.

Ah gotcha, I just did a quick search and it looks like view would be used 28 times and formSIze would also be used 28. order looks to be used much less.

Does it make sense to keep the props for those 28 cases of each? I would lean towards yes, unless you are thinking they will be removed.

jbalsas · 2020-05-18T20:25:08Z

Does it make sense to keep the props for those 28 cases of each? I would lean towards yes, unless you are thinking they will be removed.

Yeah, for 28 cases it definitely makes sense... I was just trying to err on the side of caution and add them later once we saw how a minimal API looked like. Just trying to avoid a "breaking change" if at some point we realized one of those was unused or weird. If it's clear to you, go for it, I don't have a particular concern about this.

bryceosterhaus · 2020-05-18T21:05:19Z

I was just trying to err on the side of caution and add them later once we saw how a minimal API looked like.

Totally makes sense. I think these APIs won't be too problematic for us and given the number of uses, I think it makes sense to add.

I went ahead and updated the PR:

removed order prop since it wasn't being used much
removed the sample uses of @clayui/layout in other components. Once the package is released I will go through and update our existing packages.
squashed into a single commit

Not sure If I missed anything else here in this PR, I think I covered everything though. I will mark this as ready to review.

jbalsas · 2020-05-20T07:55:50Z

packages/clay-card/src/CardWithNavigation.tsx

-				)}
-			</ClayCard.Body>
+							{title && (
+								<span className="autofit-col autofit-col-expand">


This would be a good place to use ContentCol 😂

Yeah I will go through and update our other components once a cut is released. I think if we update it before releasing @clayui/layout then CI might break for other PRs

Sounds good! 👍

jbalsas · 2020-05-20T07:56:35Z

packages/clay-card/src/CardWithNavigation.tsx

-						</span>
-						<span className="autofit-col autofit-col-expand">
-							<span className="autofit-section">
+			{(horizontal || title || description) && (


I'm looking at this in GitHub, so this might just be @wincent's most beloved UI playing tricks, but how did this change make it into this PR? Was it always here?

Yeah... probably just appears in the diff because we merged upstream... :sigh:

jbalsas · 2020-05-20T07:58:39Z

packages/clay-layout/package.json

+{
+	"name": "@clayui/layout",
+	"version": "3.0.0-alpha.1",
+	"description": "ClayLayout component",


nit, for the future: go over the descriptions in our packages. Doesn't feel like this says much about the package, what it does or why you'd want to download. Maybe we can get some copy from the Lexicon site and/or team

One of the consequences of dividing a library into 45 packages is that you have to come up with 45 descriptions. 😀

jbalsas · 2020-05-20T08:01:12Z

packages/clay-layout/src/Col.tsx

+	/**
+	 * Position in which to align column
+	 */
+	align?: 'start' | 'center' | 'end';


I'm still unsure about align... do we also have usages for this? I'm fine keeping it if you think it's safe and useful.

jbalsas · 2020-05-20T08:05:18Z

packages/clay-layout/src/Row.tsx

+}
+
+const ClayRow: React.FunctionComponent<IProps> = ({
+	align,


Same thoughts I posted about Col

jbalsas · 2020-05-20T08:05:32Z

packages/clay-layout/src/Row.tsx

+	 * Number of columns the row should contain before breaking
+	 * on to a new line
+	 */
+	numOfCol?:


Do we want to commit to this API? I think we were pushing towards autofit... and I'm not sure I've seen row-cols-X used much across the board... again, just a thought to keep our API minimal

jbalsas

Hey @bryceosterhaus, LGTM, left some additional comments here and there, but nothing major. Let's get this in and see how it looks in DXP when applied to the real components.

An additional followup would be to use them inside our own components... feels like we might be using some of these constructs ourselves in the bigger components.

Finally, I see the Storybooks, but not docs. Are we planning to add them now? or later?

bryceosterhaus · 2020-05-20T16:36:59Z

@jbalsas I'll verify some of these props in portal just to make sure. And yeah, I plan on adding the docs once this PR gets merged and before release tomorrow.

bryceosterhaus mentioned this pull request May 7, 2020

Layout util components #2421

Closed

jbalsas reviewed May 8, 2020

View reviewed changes

bryceosterhaus force-pushed the 2421 branch from 5acb8ad to caa905b Compare May 8, 2020 20:09

kresimir-coko reviewed May 11, 2020

View reviewed changes

bryceosterhaus mentioned this pull request May 11, 2020

What is Clay in relation to DXP #3214

Closed

jbalsas reviewed May 12, 2020

View reviewed changes

bryceosterhaus force-pushed the 2421 branch from 958e172 to 6244ab8 Compare May 13, 2020 20:54

jbalsas reviewed May 16, 2020

View reviewed changes

jbalsas mentioned this pull request May 18, 2020

Behaviour of autofit-float-end and autofit-col-end #3230

Closed

bryceosterhaus force-pushed the 2421 branch from 95d9293 to 2554f53 Compare May 18, 2020 21:05

bryceosterhaus marked this pull request as ready for review May 18, 2020 21:05

bryceosterhaus force-pushed the 2421 branch 2 times, most recently from befe5ad to edee160 Compare May 18, 2020 22:54

pat270 mentioned this pull request May 19, 2020

Chrome 81 Card Interactive Primary Flicker #3233

Closed

wincent mentioned this pull request May 19, 2020

LPS-113692 New Layout JSP Tags brianchandotcom/liferay-portal#88881

Closed

jbalsas reviewed May 20, 2020

View reviewed changes

jbalsas approved these changes May 20, 2020

View reviewed changes

bryceosterhaus force-pushed the 2421 branch from edee160 to 890a20b Compare May 20, 2020 18:47

feat(@clayui/layout): create low-level layout utilities

65c114e

bryceosterhaus force-pushed the 2421 branch from 890a20b to 65c114e Compare May 20, 2020 19:06

bryceosterhaus merged commit 5270404 into liferay:master May 20, 2020

edalgrin mentioned this pull request May 26, 2020

LPS-114004 Replace layout classes with new clay taglibs in account-admin-web drewbrokke/liferay-portal#1076

Closed

liferay-continuous-integration mentioned this pull request May 26, 2020

LPS-114004 Replace layout classes with new clay taglibs in account-admin-web brianchandotcom/liferay-portal#89245

Closed

	$spacers: map-deep-merge(
	(
	0: 0,
	1: (
	$spacer * 0.25,
	),
	2: (
	$spacer * 0.5,
	),
	3: (
	$spacer * 1,
	),
	4: (
	$spacer * 1.5,
	),
	5: (
	$spacer * 3,
	),
	6: (
	$spacer * 4.5,
	),
	7: (
	$spacer * 6,
	),
	8: (
	$spacer * 7.5,
	),
	),
	$spacers
	);

feat(@clayui/layout): create low-level layout utilities #3204

feat(@clayui/layout): create low-level layout utilities #3204

Conversation

bryceosterhaus commented May 7, 2020

coveralls commented May 7, 2020 • edited Loading

Pull Request Test Coverage Report for Build 110730824

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbalsas May 8, 2020 • 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

jbalsas May 8, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbalsas May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbalsas May 11, 2020 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscv-work May 13, 2020 • edited Loading

Choose a reason for hiding this comment

jbalsas left a comment

Choose a reason for hiding this comment

wincent commented May 8, 2020 • edited Loading

bryceosterhaus commented May 8, 2020

bryceosterhaus commented May 8, 2020

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

kresimir-coko May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbalsas commented May 11, 2020 • edited Loading

bryceosterhaus commented May 11, 2020

bryceosterhaus commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbalsas May 14, 2020 • 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

jbalsas commented May 12, 2020

pat270 commented May 12, 2020

bryceosterhaus commented May 13, 2020

jbalsas commented May 14, 2020

wincent commented May 14, 2020

marcoscv-work commented May 14, 2020

Choose a reason for hiding this comment

jbalsas commented May 18, 2020

coveralls commented May 7, 2020 •

edited

Loading

jbalsas May 8, 2020 •

edited

Loading

jbalsas May 8, 2020 •

edited

Loading

jbalsas May 11, 2020 •

edited

Loading

jbalsas May 11, 2020 •

edited

Loading

marcoscv-work May 13, 2020 •

edited

Loading

wincent commented May 8, 2020 •

edited

Loading

kresimir-coko May 11, 2020 •

edited

Loading

jbalsas commented May 11, 2020 •

edited

Loading

jbalsas May 14, 2020 •

edited

Loading

jbalsas commented May 18, 2020 •

edited

Loading

jbalsas May 20, 2020 •

edited

Loading