feat(@clayui/localized-input): create LocalizedInput component

[bryceosterhaus]

Hey @kresimir-coko, I made some changes on top of your PR which was mostly just refactoring and chaniging the API a bit. This way we provide a very controlled interface for users but also give flexibility if they need to do anything custom.

Could you go through and review this and then could you additionally add some unit tests to verify our functionality

Let me know if you have any questions!

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 693293c:

Sandbox	Source
cold-dew-d2iuu	Configuration
fervent-mendeleev-6zsd8	Configuration

[bryceosterhaus]

Looks like we will have to break it off into its own package to avoid circular dependencies.

@clayui/drop-down -> @clayui/form -> @clayui/drop-down

Anyone have any better ideas than just @clayui/localized-input?

The other option would be to move it to dropdown(seems a little odd to me).

[kresimir-coko]

Anyone have any better ideas than just @clayui/localized-input?

I don't think any of the packages that fit as a parent of this component, maybe someone with more knowledge of the packages might have an idea

[jbalsas]

One option would be to put it under something like @clay/advanced-form, @clay/form-extra... so we keep very basic form-like components in form and move the more complex ones (with other deps) to the advanced one...

Just a thought, though. I'm fine with @clay/input-localized too.

[bryceosterhaus]

@jbalsas would it make sense to just offer this only in portal since it seems like a pretty specific portal component?

[jbalsas]

Hey @bryceosterhaus, it certainly could, specially if we're still unsure as to how to handle this.

Text Input Localizable is however a Lexicon component, so it would be preferred if we serve it from here. Specially if the main issue is we're struggling with our package organization. I assume we'll have to figure this out anyways sooner or later...

[wincent]

I think we tend to go too far towards modularization under the illusion that people want to cherry-pick very tiny slices of Clay to use in isolation. I am not sure we have strong evidence for that though. This leads to higher overall maintenance costs and hitting obstacles like circular dependency issues. A little bit of monolith-ization wouldn't hurt at this point.

[bryceosterhaus]

@wincent are you suggesting we offer just an all encompassing package instead of granular packages? Or what exactly do you see as steps towards "monolith-ization"?

@jbalsas my main thought was I couldn't really imagine it being used in our other projects besides portal, which is why I thought maybe only adding there makes sense. As a side benefit, we wouldn't have to deal with how we deliver it in clay 😁

[wincent]

@wincent are you suggesting we offer just an all encompassing package instead of granular packages? Or what exactly do you see as steps towards "monolith-ization"?

I wouldn't race to put everything in a single package (if we had a time machine, we could go back to the beginning of v3 and discuss whether that's a desirable path), but where we are now, I'm just saying that when you run into cyclic dependencies between a group of 2 or 3 packages, that might be a hint that they should be aggregated into one (or two). Sorry if that's vague/abstract, but I don't know the exact nature of the cycle you're talking about here.

In general terms, the two main ways to break dependency cycles — eg an A → B → A cycle — are, either:

• Identify common substrate that both A and B need, extract it into C, and have A and B each depend on C.
• Combine A and B into a single module AB, so that they can freely interoperate (in the case of JS, because functions are hoisted, you can define A and B in the same module and they can call each other irrespective of the order in which they are defined).

What does that mean in this concrete case? I don't know the details, but the first approach is scary because it increases the number of packages and may be hard. In general modular is hard, monolith is easy.

I know we've talked about modularization in the past, but the only reference I can find in a quick search is this one where we talk about "web apps are forms" (with the implication that ultimately everything winds up in @clayui/forms).

In the long term, I think the most maintainable future version of Clay probably has quite a large "forms" package and a small number of satellite packages.

@jbalsas my main thought was I couldn't really imagine it being used in our other projects besides portal, which is why I thought maybe only adding there makes sense. As a side benefit, we wouldn't have to deal with how we deliver it in clay 😁

I worry about this line of thinking. At the moment, Clay has a really simple definition: it is an implementation of the Lexicon design system. What you're proposing moves the definition towards, "An implementation of the Lexicon design system, except for the bits that are rarely used or whose complication makes us reluctant to commit to maintaining them inside Clay itself". I think the simple definition is better.

The other point thing that I am afraid of the risk that we invest too much effort in making Clay into a general purpose UI framework by optimizing for use cases outside of portal, losing sight of the reality that Lexicon exists for the benefit of portal, Clay exists for the benefit of portal, and we all have jobs and earn salaries because of portal etc.

[bryceosterhaus]

I worry about this line of thinking. At the moment, Clay has a really simple definition: it is an implementation of the Lexicon design system. What you're proposing moves the definition towards, "An implementation of the Lexicon design system, except for the bits that are rarely used

From what I remember in initial conversations is we actually did err more on the side of your second example in the sense that we didn't want to have overly specific components and API that was built just for liferay-portal. In my understanding the goal has been to implement the lexicon design system in a generic way so that it is not confined to portal specifically. This helps adoption in other products such as analytics cloud and also helps us maintain a more consumable library as a whole. LocalizedInput seemed like a specific API of an Input that is made specifically for portal, not a general component consumed in multiple apps. And this is why I think we have talked about the middle layer of interfacing clay components for portals specific needs, which lives in liferay-portal. (There was an issue we talked about this on but I can't seem to find it...).

or whose complication makes us reluctant to commit to maintaining them inside Clay itself"

I don't think we should be reluctant to add components that are complicated, but I am reluctant to add and maintain something that is built for one specific case and product. We have also talked about this in how we generally want to see a component used across multiple products or apps before generalizing it in clay.

The other point thing that I am afraid of the risk that we invest too much effort in making Clay into a general purpose UI framework by optimizing for use cases outside of portal, losing sight of the reality that Lexicon exists for the benefit of portal, Clay exists for the benefit of portal, and we all have jobs and earn salaries because of portal etc.

If our goal for Clay is to only use it for portal's benefit, I would be curious why we are building it in isolation and not just migrating this whole project into the liferay-portal repo. IMO I think we should aim for the clay repo to model a general purpose UI framework and then any highly specific implementations to portal would be added over there by either us or someone else. I agree our team should have a goal of benefitting portal, but I don't think the clay repo's primary purpose is portal.

This seems to be divulging into a whole other issue than just the LocalizedInput. Maybe a new issue should be created to discuss this?

[jbalsas]

Hey @bryceosterhaus!

I think your comment and recolection from our conversations is pretty accurate. There's one distinction I'd like to make, though, regarding this:

[...] I am reluctant to add and maintain something that is built for one specific case and product. We have also talked about this in how we generally want to see a component used across multiple products or apps before generalizing it in clay.

We often fall into this cognitive bias. We equate Liferay Portal and Analytics as equal "products" or "apps" when in reality, Portal is 10 times the size of Analytics and composed of several apps the size of Analytics.

For example, considering only the biggest functional Apps in Portal, we have:

• Forms
• PageEditor
• Segmentation
• Blogs
• AppBuilder
• Commerce
• ...

All these are full-fledged apps that rival in size and complexity with Analytics. I get the feeling we often fall in the trap to just bundle them and say Portal is just one product/app, when in reality it's a combination of several.

In this case, for example, the InputLocalized pattern is used across all those apps, granting (in my opinion) the status of general component. Had it only been used in Forms, we would have asked that team to implement it on their own first.

I agree we can continue this conversation elsewhere, but just wanted to take the chance to make this clarification in case it helps steer the decision...

[bryceosterhaus]

Hey @jbalsas thanks for the reminder of the many apps in liferay-portal. However, I bring up analytics not as to say its a comparable product to liferay-portal or the many products in portal, but I often use it as an example since its sort of at the other end of the spectrum. It's in its own repo and behaves more like a traditional React app, whereas the apps within portal all share the same context and have the same access to the internals of liferay-portal. Which I think continues to raise the question, if LocalizedInput(or any other component) is only ever intended to be used in liferay-portal, should that live in the liferay-portal repo instead of being generalized and brought to clay? I think there is benefit in having it live in portal, since we don't necessarily have to generalize it as much as we would want to in clay.

the InputLocalized pattern is used across all those apps, granting (in my opinion) the status of general component. Had it only been used in Forms, we would have asked that team to implement it on their own first.

I also did a cursory search in liferay-portal and I see that the LocalizedInput is only used in segments right now. Do you happen to know where they are used in other apps or how they intend to be used? I think if we can see how it's going to be used, it might give us more insight of how we can generalize it.

In the end, I'm not necessarily opposed to having @clayui/localized-input, I am just hesitant of throwing components portal specific components into clay that may not be used anywhere outside of the liferay-portal repo since its likely better that those components would be better off living in the source of liferay-portal.

[wincent]

I think there is benefit in having it live in portal, since we don't necessarily have to generalize it as much as we would want to in clay

I would disagree that being in Clay means that something has to be general. If Lexicon defines something specific, that seems reason enough for the implementation to be specific. (I know this comes back to the earlier point about Clay !== Lexicon, but I actually think it should be === Lexicon because if we don't leverage the design system everywhere we can, we're really not getting bang for our buck. If we keep making sub-products that deviate from Lexicon, to me that's an indication that we should evolve Lexicon, not that we should create a bunch of parallel worlds to it and then expect Clay to somehow serve them all.)

[jbalsas]

I also did a cursory search in liferay-portal and I see that the LocalizedInput is only used in segments right now. Do you happen to know where they are used in other apps or how they intend to be used? I think if we can see how it's going to be used, it might give us more insight of how we can generalize it.

Things in Portal aren't always where you think they would :D

Segments is the first team that had to use such a component from React. Since we didn't have it available at the time, we kindly asked that they built it themselves.

This week, the Forms team had to implement LocalizableText which @matuzalemsteles and @diegonvs are currently migrating to React... this might be our second React ocurrence.

Our more extensive usage comes from the legacy InputLocalizedTag that is currently implemented in jsp and AUI

The liferay-ui:input-localized tag is occasionally used directly... but more importantly, it's called from aui:input whenever the input field is localizable.

[jbalsas]

However, I bring up analytics not as to say its a comparable product to liferay-portal or the many products in portal, but I often use it as an example since its sort of at the other end of the spectrum

I think this is very valuable context that we indeed need to pay attention to. However, if we had to remove anything they don't use (or don't see themselves using), we might as well move clay into liferay-portal as you suggested 😂.

I think @wincent also points to the necessary convergence between those apps and Lexicon. I now for a fact that's slowly happening, and Clay will be there to help those apps UX converge.

Finally, as far as this component go, I think there's a valid balance to be reached out here.

• Clay offers the main UI components to compose and leverage the pattern in a simple (robust, performant, and accessible) way (that's how I tried to steer @kresimir-coko to implement it)
• Portal will use those pieces and implement the internal business logic that might be unique to that platform

[bryceosterhaus]

This week, the Forms team had to implement LocalizableText which @matuzalemsteles and @diegonvs are currently migrating to React... this might be our second React ocurrence.
Our more extensive usage comes from the legacy InputLocalizedTag that is currently implemented in jsp and AUI

Ah I see. If thats the case and we plan on using it frequently, probably makes sense for it to be its own package.

Finally, as far as this component go, I think there's a valid balance to be reached out here.

• Clay offers the main UI components to compose and leverage the pattern in a simple (robust, performant, and accessible) way (that's how I tried to steer @kresimir-coko to implement it)
• Portal will use those pieces and implement the internal business logic that might be unique to that platform

Totally agree, which is sort of my reason for initially raising the question of where this code should live. Initially LocalizedInput looked more like a composition of pieces(input & dropdown) and wired up minimal business logic for the two to interact.

And yeah both of you are right in that this really boils down to how we wan't to define Clay(more specifically the clay components) and if it should be Clay === Lexicon or do we define Clay components as a subset of "lexicon." And I use lexicon in quotes since even that can be hard to have an accurate definition for. Not totally sure how we would take next steps in defining this.

Anyways, for this component, it seems like you guys see value in having this component served from the clay repo rather than liferay-portal so I decided to extract it to its own component. I did that rather than lump it into @clayui/form since form probably shouldn't have a dependency on drop-down. I've pushed the changes to this branch, let me know what you think!

[kresimir-coko]

Is this the approach we'll have going forward with components that require aria-labels?
I noticed while doing the accessibility that some components use this approach.

[bryceosterhaus]

Yeah this is a common pattern for us to add since we need to add the ability for localization. Date picker, time picker, and color picker all use it right now I believe

[kresimir-coko]

Is this always the superior choice compared to what I did by defining ClayComponentWithState above the component?

[bryceosterhaus]

Not necessarily always, but I think its best if we avoid over abstraction and wait until it's needed. In this case it wasn't super necessary to abstract since we didn't use it in multiple places and it also didn't necessarily help the readability or complexity of the code.

[wincent]

it seems like you guys see value in having this component served from the clay repo rather than liferay-portal

Independently of the big-picture ontological question of what Clay is 😂, a very tangible benefit of having stuff in this repo is that we can use TypeScript and benefit from all the other stuff that comes with this repo (extensive CI checks, place to store documentation etc).

[coveralls]

Pull Request Test Coverage Report for Build 4680

• 15 of 17 (88.24%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 78.536%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/clay-localized-input/src/index.tsx	15	17	88.24%

Totals
Change from base Build 4679:	0.1%
Covered Lines:	2297
Relevant Lines:	2726

---

💛 - Coveralls

[bryceosterhaus]

Once this approved, i'll go ahead and squash into a single commit so that the --conventional-commits doesn't get wonky with the add+remove to @clayui/form

[bryceosterhaus]

Merging since tests pass. I also spun off a new issue for docs, #3104