feat(@clayui/management-toolbar): add high-level ClayManagementToolbar component #2225

matuzalemsteles · 2019-07-25T23:00:58Z

I ended up following the same idea we did in v2 and I left the same API for most, yet I'm worried about the bunch of props we have to cover all the use cases it supports, that's one of my concerns when we have high-level components that are very large and with many use cases like this.

Leave your thoughts on this component if we will follow up with any different ideas for it.

There is still some testing and documentation to add to this component, so once we are fine with the API I will add and also has some improvements to make in this component, such as using sub, create DropDownWithAdvancedItems component.

Tests
Docs

packages/clay-management-toolbar/src/DefaultState.tsx

diegonvs · 2019-07-26T15:58:17Z

packages/clay-management-toolbar/src/Item.tsx

+import React from 'react';
+
+interface IProps extends React.HTMLAttributes<HTMLLIElement> {
+	expand?: boolean;


should this could be here? We aren't using this property on Item.

Oh yeah, I just forgot to remove it. Thanks

diegonvs · 2019-07-26T15:59:24Z

packages/clay-management-toolbar/src/ItemList.tsx

+	expand?: boolean;
+}
+
+const ItemList: React.FunctionComponent<IProps> = ({children, expand}) => (


Suggested change

const ItemList: React.FunctionComponent<IProps> = ({children, expand}) => (

const ItemList: React.FunctionComponent<IProps> = ({children, expand}) => (

const ItemList: React.FunctionComponent<IProps> = ({children, expand = false}) => (

This does not have much need here since undefined in the condition is false too so it would be something more aesthetic.

diegonvs · 2019-07-26T16:00:46Z

packages/clay-management-toolbar/src/ResultsBar.tsx

+				<ResultsBarItem expand={!(filterLabels.length > 0)}>
+					<span className="component-text text-truncate-inline">
+						<span className="text-truncate">
+							{`${totalItems} results for "`}


What do you think about wrap this text with a sub?

Yeah, I haven't added sub yet, that's one of the things I said in the PR description.

diegonvs · 2019-07-26T16:02:32Z

packages/clay-management-toolbar/src/SearchForm.tsx

+	...otherProps
+}) => {
+	const content = (
+		<form {...otherProps} method={method} role="search">


Would be a good idea ship this code to ClayForm?

Not sure about that, I still don't see a real need to create a component to render just the form.

diegonvs · 2019-07-26T16:04:50Z

packages/clay-management-toolbar/src/__tests__/index.tsx

+describe('ClayManagementToolbar', () => {
+	afterEach(cleanup);
+
+	it('renders', () => {


Need we to include more interaction tests? Especially for event callbacks

Yeah, there's still a lot to work on testing, I'll add after we are fine with the component approach, I mentioned above in the PR description.

coveralls · 2019-07-26T18:03:17Z

Pull Request Test Coverage Report for Build 3201

95 of 104 (91.35%) changed or added relevant lines in 12 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.7%) to 78.674%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/clay-management-toolbar/src/ActiveState.tsx	13	14	92.86%
packages/clay-management-toolbar/src/CreationMenu.tsx	13	14	92.86%
packages/clay-management-toolbar/src/SearchForm.tsx	6	7	85.71%
packages/clay-management-toolbar/src/SearchInput.tsx	6	7	85.71%
packages/clay-management-toolbar/src/DefaultState.tsx	18	20	90.0%
packages/clay-shared/src/LinkOrButton.tsx	4	7	57.14%

Totals
Change from base Build 3193:	0.7%
Covered Lines:	1909
Relevant Lines:	2261

💛 - Coveralls

fabio-d-m · 2019-08-01T14:10:17Z

Hello @matuzalemsteles :) in commerce we have a scenario where we need to use a custom handmade dropdown in order to show and apply custom filters.

Do you think it would be possibile (and compliant with clay) passing a custom filter component as a prop?

cc: @marco-leo , @jbalsas

matuzalemsteles · 2019-08-01T16:28:57Z

Oh thanks @FabioDiegoMastrorilli for bringing this use case, I was thinking about it yesterday when I was also looking at the App Builder use case. I'll think of something, maybe instead of being an API for rendering DropDown items maybe we can change For a filterRender that accepts the DropDown element, I find it easier to be flexible. @bryceosterhaus thoughts?

bryceosterhaus · 2019-08-05T20:57:00Z

@matuzalemsteles if customization is defined by lexicon, I think we can add the API. If it doesn't fall under lexicon's specification, we should probably push users to utilize the low-level layer

marco-leo · 2019-08-05T21:01:03Z

Hi @bryceosterhaus @matuzalemsteles we do need this extension point for Commerce, will save us lot of future works, I will make sure this is added as Lexicon extension point.

Thanks

matuzalemsteles · 2019-08-06T21:12:47Z

hey @bryceosterhaus I think we can allow devs to customize DropDown content by passing filterRender for example, I think if we force devs to use low levels they will have a lot of work to do with management toolbar, just to add more things or modifying, this component may change a lot and be more flexible, so we can focus and not get as stuck as before.

One day I saw the App Builder team discussing how it would be best to create just a part of the management toolbar for their use cases and something that I realized we had spent a lot of time discussing about the state of the art. of how to make the component or structure instead of worrying about other things that may be more important in your product, since this is a base part of portlets for example. So I think we can start to be a little more flexible and adjust, it's probably something Lexicon will be doing now and so we can make people happier...

drakonux · 2019-08-07T13:57:53Z

Hi everyone! We have received a request from team-commerce to evaluate an improvement in the management toolbar based on the scenario they currently have.

So, I'm working on that now. I'll keep you posted.

matuzalemsteles · 2019-08-07T17:22:56Z

Oh thanks @drakonux, so this would probably be on On Hold?! I think we can go ahead with this and then we can apply the changes that @drakonux requests. @bryceosterhaus?

bryceosterhaus

Noticed a handful of things. Primarily random no-ops that seemed unnecessary and also a need to remove explicit english language keys and replace with configurable messages.

packages/clay-form/src/InputWithAutocomplete.tsx

packages/clay-management-toolbar/package.json

packages/clay-management-toolbar/src/ActiveState.tsx

packages/clay-management-toolbar/src/DefaultState.tsx

packages/clay-management-toolbar/src/ResultsBar.tsx

packages/clay-management-toolbar/src/SearchForm.tsx

packages/clay-management-toolbar/src/index.tsx

bryceosterhaus · 2019-08-07T18:24:20Z

@matuzalemsteles yeah I think we can keep going ahead with this and add extensibility later whenever it gets baked out a bit more.

packages/clay-shared/src/LinkOrButton.tsx

matuzalemsteles · 2019-08-12T19:09:15Z

@bryceosterhaus update.

bryceosterhaus

@matuzalemsteles unless I am missing something here, I'm still not sure all of these no-op values for callbacks make sense. We also seem to use both callbacks with the addition of show*Button props. I think we can remove all no-ops, and then use the callbacks for inferring the show*Button values. Since we use TS, we are able to enforce callbacks via required props or conditionally render if the prop exists.

One question I ask myself to help decide no-ops is "is this callback required for functionality." For example, in the case of a checkbox, we wouldn't ever want the end result to show a checkbox with a no-op for onChange. We either want the checkbox to be functional, or we don't show it at all. Same goes for most buttons and onChange type callbacks.

packages/clay-drop-down/src/index.tsx

packages/clay-form/src/InputWithAutocomplete.tsx

packages/clay-management-toolbar/src/ActiveState.tsx

packages/clay-management-toolbar/src/DefaultState.tsx

packages/clay-shared/src/LinkOrButton.tsx

matuzalemsteles · 2019-08-13T17:39:36Z

@matuzalemsteles unless I am missing something here, I'm still not sure all of these no-op values for callbacks make sense. We also seem to use both callbacks with the addition of showButton props. I think we can remove all no-ops, and then use the callbacks for inferring the showButton values. Since we use TS, we are able to enforce callbacks via required props or conditionally render if the prop exists.

One question I ask myself to help decide no-ops is "is this callback required for functionality." For example, in the case of a checkbox, we wouldn't ever want the end result to show a checkbox with a no-op for onChange. We either want the checkbox to be functional, or we don't show it at all. Same goes for most buttons and onChange type callbacks.

In this regard, no-ops callbacks are necessary for people to control actions, I would recommend you to see how it is used within the Portal...

In the particular case of checkbox onChange it is necessary since checkbox action is to select items from table, card and list for example.

About the show * props I like that since it is so much more intuitive, it makes sense to use only callbacks to render the elements since they are needed here but it does not seem so intuitive. This component can behave in many ways and has several use cases.

bryceosterhaus · 2019-08-14T22:03:36Z

In this regard, no-ops callbacks are necessary for people to control actions, I would recommend you to see how it is used within the Portal

I'm still not totally sure what you mean by this and having trouble wrapping my head around this. Could you link me to a specific instance where this is needed? In particular, a place where the no-op is necessary for user interaction.

in the particular case of checkbox onChange it is necessary

I understand that onChange is necessary for checkbox to work, but the question I am raising is why its necessary or helpful to provide () => {} to that component. If you add that callback, the checkbox doesn't actually function right?

About the show * props I like that since it is so much more intuitive

I'd push back on this primarily due to its "tight coupling." If we use both props, those two props are tightly coupled and would never be used separately. You would never just use showSelectAllButton without onSelectAllButton or vice versa, onSelectAllButton without showSelectAllButton. Generally I would prefer any tightly coupled props, especially when one of them is a boolean.

matuzalemsteles · 2019-08-15T15:15:00Z

I'm still not totally sure what you mean by this and having trouble wrapping my head around this. Could you link me to a specific instance where this is needed? In particular, a place where the no-op is necessary for user interaction.

I don't have an exact Portal link or instance, you might want to look at Documents and Media, Forms... all main pages use the Management Toolbar. But especially I don't know if the code would be the best to show, because the use case there is different, they are all taglibs and it's metal, I think all of that makes more sense there. Since you have no callback props and only events.

I understand that onChange is necessary for checkbox to work, but the question I am raising is why its necessary or helpful to provide () => {} to that component. If you add that callback, the checkbox doesn't actually function right?

All this becomes necessary since I followed the idea of having props show* because I am not inferring the callbacks on conditionals to decide if I will render the element, the question is, are we going to remove the show* props? If yes we essentially don't need this in callbacks, the hesitation I'm having all about is using show* props or not? will this be more beneficial for the developer? Will he have to guess if using a callback is what defines rendering the component? callbacks with () => {} are just consequences of all this.

I'd push back on this primarily due to its "tight coupling." If we use both props, those two props are tightly coupled and would never be used separately. You would never just use showSelectAllButton without onSelectAllButton or vice versa, onSelectAllButton without showSelectAllButton. Generally I would prefer any tightly coupled props, especially when one of them is a boolean.

Yes, I agree with you about this, it can be a redundancy but in the end I am wondering if this is all worth it in order to be the best for the developer.

To end our conversation and not extend so long to finish the deadline... I will remove the show* props and consequently callback no-ops and we can test how this will work, I will add in the conditionals all the elements that are needed to render the element so as not to be in charge of just the callback.

bryceosterhaus · 2019-08-15T18:14:51Z

Sounds good to me. We could further discuss this in an ADR to go more in depth on show* props, because if we go that route we will likely want to apply it across the whole codebase to keep consistency(for example, Alerts would likely need onClose and showCloseButton).

matuzalemsteles · 2019-08-15T19:17:44Z

Sounds good to me. We could further discuss this in an ADR to go more in depth on *show props, because if we go that route we will likely want to apply it across the whole codebase to keep consistency(for example, Alerts would likely need onClose and showCloseButton).

Yeah, I think in the end what will tell you what is the best is the devs, we could try to collect feedback to make that decision. Because if you follow some of the two we will have some exchange.

bryceosterhaus · 2019-08-15T19:25:59Z

@matuzalemsteles yeah thats a great point. Maybe we could try to put together a few users as a case study and see how they use it and what they need from it. Or maybe even just post a feedback form on Loop and see if we can collect any data from users.

matuzalemsteles · 2019-08-15T21:10:29Z

@matuzalemsteles yeah thats a great point. Maybe we could try to put together a few users as a case study and see how they use it and what they need from it. Or maybe even just post a feedback form on Loop and see if we can collect any data from users.

Yeah that's a great idea!

matuzalemsteles · 2019-08-20T14:02:36Z

hey @bryceosterhaus update. I removed the show* props and added LinkOrButton, let me know if you see anything else.

bryceosterhaus

Looks good. Just had minor things I noted, not a huge deal before merge though.

packages/clay-management-toolbar/src/ActiveState.tsx

bryceosterhaus

lgtm. want to add some more tests and docs?

matuzalemsteles · 2019-08-21T00:10:33Z

@bryceosterhaus I will still add documents and tests for this component, I am working on it.

… to pass to ClayAutocomplete

matuzalemsteles · 2019-08-21T01:18:16Z

I'm just pushing formatted commits, tomorrow I'll be working on tests and docs.

drakonux · 2019-08-21T09:01:16Z

Hi everyone! We have received a request from team-commerce to evaluate an improvement in the management toolbar based on the scenario they currently have.

So, I'm working on that now. I'll keep you posted.

Hi everyone, just for your information based on the request team-commerce did a couple of weeks ago. I've created a new issue requesting two extension points in the Management Toolbar.

matuzalemsteles · 2019-08-21T23:14:57Z

@bryceosterhaus I added the docs and tests, let me know if you see anything.

bryceosterhaus

LGTM

diegonvs suggested changes Jul 26, 2019

View reviewed changes

bryceosterhaus requested changes Aug 7, 2019

View reviewed changes

matuzalemsteles force-pushed the issue-1945 branch from 4a63784 to a4118df Compare August 12, 2019 19:02

matuzalemsteles commented Aug 12, 2019

View reviewed changes

packages/clay-shared/src/LinkOrButton.tsx Outdated Show resolved Hide resolved

bryceosterhaus reviewed Aug 12, 2019

View reviewed changes

bryceosterhaus mentioned this pull request Aug 16, 2019

[Discussion] High-level vs low-level component API #2275

Closed

bryceosterhaus reviewed Aug 20, 2019

View reviewed changes

packages/clay-management-toolbar/src/ActiveState.tsx Show resolved Hide resolved

bryceosterhaus reviewed Aug 21, 2019

View reviewed changes

matuzalemsteles added 3 commits August 20, 2019 21:57

feat(@clayui/management-toolbar): set up ClayManagementToolbar structure

15a66cb

chore(@clayui/label): add new type unstyled to displayType

a079f9f

chore(@clayui/button): add docs to ButtonWithIcon

828b747

matuzalemsteles added 2 commits August 20, 2019 22:07

fix(@clayui/form): add containerRenderer API on InputWithAutocomplete…

ad1e49c

… to pass to ClayAutocomplete

feat(@clayui/shared): add LinkOrButton component for use internal

63371b0

matuzalemsteles force-pushed the issue-1945 branch from 9616dfd to a153c29 Compare August 21, 2019 01:17

matuzalemsteles mentioned this pull request Aug 21, 2019

Management Toolbar: New extension points #2292

Closed

matuzalemsteles added 2 commits August 21, 2019 20:10

feat(@clayui/management-toolbar): add ClayManagementToolbar component

fdab7b8

feat(@clayui/management-toolbar): add basic rendering tests

2a0869b

matuzalemsteles force-pushed the issue-1945 branch from a153c29 to 2a0869b Compare August 21, 2019 23:13

matuzalemsteles mentioned this pull request Aug 22, 2019

feat(@clayui/tabs): add low-level Tabs component #2317

Merged

bryceosterhaus reviewed Aug 22, 2019

View reviewed changes

matuzalemsteles merged commit a5d5ad8 into liferay:master Aug 22, 2019

matuzalemsteles mentioned this pull request Aug 22, 2019

High-level Management Toolbar component #2155

Closed

	const ItemList: React.FunctionComponent<IProps> = ({children, expand}) => (
	const ItemList: React.FunctionComponent<IProps> = ({children, expand}) => (
	const ItemList: React.FunctionComponent<IProps> = ({children, expand = false}) => (

feat(@clayui/management-toolbar): add high-level ClayManagementToolbar component #2225

feat(@clayui/management-toolbar): add high-level ClayManagementToolbar component #2225

Conversation

matuzalemsteles commented Jul 25, 2019 • 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

coveralls commented Jul 26, 2019 • edited Loading

Pull Request Test Coverage Report for Build 3201

💛 - Coveralls

fabio-d-m commented Aug 1, 2019 • edited Loading

matuzalemsteles commented Aug 1, 2019

bryceosterhaus commented Aug 5, 2019

marco-leo commented Aug 5, 2019

matuzalemsteles commented Aug 6, 2019

drakonux commented Aug 7, 2019

matuzalemsteles commented Aug 7, 2019

bryceosterhaus left a comment

Choose a reason for hiding this comment

bryceosterhaus commented Aug 7, 2019

matuzalemsteles commented Aug 12, 2019

bryceosterhaus left a comment

Choose a reason for hiding this comment

matuzalemsteles commented Aug 13, 2019

bryceosterhaus commented Aug 14, 2019

matuzalemsteles commented Aug 15, 2019

bryceosterhaus commented Aug 15, 2019 • edited Loading

matuzalemsteles commented Aug 15, 2019

bryceosterhaus commented Aug 15, 2019

matuzalemsteles commented Aug 15, 2019

matuzalemsteles commented Aug 20, 2019

bryceosterhaus left a comment

Choose a reason for hiding this comment

bryceosterhaus left a comment

Choose a reason for hiding this comment

matuzalemsteles commented Aug 21, 2019

matuzalemsteles commented Aug 21, 2019

drakonux commented Aug 21, 2019

matuzalemsteles commented Aug 21, 2019

bryceosterhaus left a comment

Choose a reason for hiding this comment

matuzalemsteles commented Jul 25, 2019 •

edited

Loading

coveralls commented Jul 26, 2019 •

edited

Loading

fabio-d-m commented Aug 1, 2019 •

edited

Loading

bryceosterhaus commented Aug 15, 2019 •

edited

Loading