COMMERCE-5449 | COMMERCE-5450 Inline editing and entity creation via dataset display #700
Conversation
|
CI is automatically triggering the following test suites:
|
✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: COMMERCE-5450 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#4861 |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#7720 |
|
Thanks a lot, @FabioDiegoMastrorilli, looks nice, but feels like it might take a little bit to review! 😂 Going to assign this to @bryceosterhaus for monday, but @markocikos, @julien, @carloslancha, feel free to take a look too, please! 🙏 |
fabio-d-m
force-pushed
the
COMMERCE-5450
branch
from
372488f
to
023fa18
Compare
|
Going to have a look at the code changes. |
modules/apps/commerce/commerce-frontend-js/test/dev/components/DatasetDisplay.js
Outdated
Show resolved
Hide resolved
modules/apps/commerce/commerce-frontend-js/test/dev/components/DatasetDisplay.js
Outdated
Show resolved
Hide resolved
|
|
||
| window.defaultFetch = fetch; | ||
|
|
||
| window.fetch = (resource, {headers, ...init}) => { |
There was a problem hiding this comment.
Couldn't you use fetch from frontend-js-web and pass those Headers?
There was a problem hiding this comment.
Julien on this one I preferred keeping it as it is. This file contains some utilities we place a static env to replace standard liferay ones (like Liferay.Language.get or themeDisplay.getLanguageId) and it doesn't get compiled.
Its only purpose is avoiding to write exceptions (that are aimed to make everything working in static env) in code which will be deployed
...rontend-taglib-clay/src/main/resources/META-INF/resources/data_set_display/DataSetDisplay.js
Show resolved
Hide resolved
| ('0' + dateInstance.getHours()).slice(-2) + | ||
| ':' + | ||
| ('0' + dateInstance.getMinutes()).slice(-2); | ||
|
|
There was a problem hiding this comment.
I'd also use template literals here instead of concatenation
...aglib-clay/src/main/resources/META-INF/resources/data_set_display/utilities/dataRenderers.js
Show resolved
Hide resolved
...rontend-taglib-clay/src/main/resources/META-INF/resources/data_set_display/DataSetDisplay.js
Outdated
Show resolved
Hide resolved
| sidePanelId || 'support-side-panel-' + getRandomId() | ||
| ); | ||
| const [delta, setDelta] = useState( | ||
| pagination.initialDelta || pagination.deltas[0].label |
There was a problem hiding this comment.
What if pagination is not defined or pagination.deltas is empty?
There was a problem hiding this comment.
I set it in DataSetDisplay.defaultProps so it's pretty safe I hope
...rontend-taglib-clay/src/main/resources/META-INF/resources/data_set_display/DataSetDisplay.js
Outdated
Show resolved
Hide resolved
...-taglib/frontend-taglib-clay/src/main/resources/META-INF/resources/data_set_display/entry.js
Outdated
Show resolved
Hide resolved
...-taglib-clay/src/main/resources/META-INF/resources/data_set_display/views/table/TableCell.js
Outdated
Show resolved
Hide resolved
...-taglib-clay/src/main/resources/META-INF/resources/data_set_display/views/table/TableCell.js
Outdated
Show resolved
Hide resolved
...-taglib-clay/src/main/resources/META-INF/resources/data_set_display/views/table/TableCell.js
Outdated
Show resolved
Hide resolved
...lib-clay/src/main/resources/META-INF/resources/data_set_display/views/table/TableHeadCell.js
Outdated
Show resolved
Hide resolved
...lib-clay/src/main/resources/META-INF/resources/data_set_display/views/table/TableHeadCell.js
Outdated
Show resolved
Hide resolved
...rontend-taglib-clay/src/main/resources/META-INF/resources/data_set_display/DataSetDisplay.js
Outdated
Show resolved
Hide resolved
...rontend-taglib-clay/src/main/resources/META-INF/resources/data_set_display/DataSetDisplay.js
Outdated
Show resolved
Hide resolved
| @@ -75,29 +78,30 @@ function DataSetDisplay({ | |||
| sorting: sortingProp, | |||
| style, | |||
| }) { | |||
| const {apiURL} = useContext(AppContext); | |||
|
|
|||
There was a problem hiding this comment.
There are a handful of functions declared here in the "render" of this component. For the sake of performance and to avoid re-creating these functions repeatedly, you might want to go through and try to utilize useCallback and useMemo for some of these.
There was a problem hiding this comment.
I'd be very careful about adding useCallback or useMemo just to avoid "re-creating functions repeatedly". Note that in order to make use of either, you are instantiating a new function every time through your render anyway; eg:
There was a problem hiding this comment.
Whoops, accidentally focused the "Comment" button and hit "Enter" before I finished entering the comment. Updated comment incoming...
There was a problem hiding this comment.
I'd be very careful about adding useCallback or useMemo just to avoid "re-creating functions repeatedly". Note that in order to make use of either, you are instantiating a new function every time through your render anyway; eg: (function chosen at random, but it's just for illustration):
function DataSetDisplay() {
// stuff
function updateDataSetItems(dataSetData) {
setTotal(dataSetData.totalCount);
updateItems(dataSetData.items);
}
// etc
}vs:
function DataSetDisplay() {
// stuff
const updateDataSetItems = useCallback((dataSetData) => {
setTotal(dataSetData.totalCount);
updateItems(dataSetData.items);
}, [...]);
// etc
}The first version is re-instantiating a function object on every render, but so is the second one (the arrow function). In addition, it is paying the overhead of calling useCallback on every render, and it is allocating a dependencies array on every render. useCallback itself is going to incur the cost of comparing the items in the dependency array (using Object.is()), as well as the memory overhead of any related book-keeping details that React is maintaining internally.
Now, I'm not saying "never use useCallback or useMemo"; I'm just saying, make sure you have a good reason to use them. A non-exhaustive list of good reasons for using them is:
- Use
useMemoto avoid repeatedly recomputing an intermediate value that is expensive to evaluate (note: in order for this to pay off, the value shouldn't be changing on every render and the cost of determining equality should be cheaper than just redoing the calculation — remember that you're paying the cost of the equality comparison plus the overhead of the function call into React internals plus whatever internal book-keeping React might be doing). In many cases, it's cheaper to just re-do the computation every time. - Use
useCallbackwhen you want to maintain a stable identity for a callback function that you will be passing down to children; this can be important when the callback ends up being a member of auseEffectdependencies array in the child (or grandchild, or great-grandchild etc). If the callback hasn't really changed, you may find that failure to useuseCallbackcauses children to re-run effects unnecessarily because they think something has changed when it really hasn't. This is another context-dependent thing: sometimes you know everything about your children subtree and you can know deterministically thatuseCallbackis required; at other times you are writing "library" code and you have no idea what your children are going to be doing, so you may want to err on the side of being defensive and usinguseCallback"just in case".
Another angle on this: https://kentcdodds.com/blog/usememo-and-usecallback/
Performance optimizations ALWAYS come with a cost but do NOT always come with a benefit.
There was a problem hiding this comment.
Made this issue for us to put something about these hooks in the guidelines: liferay/liferay-frontend-projects#383
| @@ -194,11 +249,27 @@ function ActionsDropdownRenderer({actions, itemData, itemId}) { | |||
| }, []) | |||
| : []; | |||
|
|
|||
| if (alwaysOn && (!formattedActions || !formattedActions.length)) { | |||
There was a problem hiding this comment.
Same as comment above about using a descriptive variable
...main/resources/META-INF/resources/data_set_display/data_renderers/ActionsDropdownRenderer.js
Outdated
Show resolved
Hide resolved
...lib-clay/src/main/resources/META-INF/resources/data_set_display/views/table/TableHeadCell.js
Outdated
Show resolved
Hide resolved
...lib-clay/src/main/resources/META-INF/resources/data_set_display/views/table/TableHeadCell.js
Outdated
Show resolved
Hide resolved
| <ClayIcon | ||
| className={classNames( | ||
| 'sorting-icon', | ||
| sortingMatch?.direction === 'asc' && 'active' |
There was a problem hiding this comment.
Same as above, it's probably gonna be a bit more readable to use the object syntax for classNames. Note that you can use both an object and a string. So the first argument would be 'sorting-icon' and the second arg would be an object for toggling 'active'
There was a problem hiding this comment.
Only on this one and the other one below I would prefer keeping them as they are. Passing both a string and an object to classnames looks a bit less readable (or maybe I'm just not used to it). active also will not be highlighted as a string due to our linter an it looks a bit strange to me:
className={classNames(
'sorting-icon',
{
active: sortingMatch?.direction === 'asc'
}
)}
Btw if there are some standards we have to follow in this cases, not problems at all :)
There was a problem hiding this comment.
I don't think we have any standards written down, and we probably won't either unless we start to see classNames() usage "spiralling into chaos" 😂.
I don't have an opinion on this strong enough to make me want to comment, but seeing as the thread is already open, the way I usually tend to write these things to avoid the feeling of impedance mismatch you get when mixing string and object is to just use object for everything:
<ClayIcon
className={{
active: sortingMatch?.direction === 'asc',
'sorting-icon': true,
}}
/>but like all things, it's a trade-off; like you say @FabioDiegoMastrorilli, you lose the string highlighting of the active classname. We should be able to get it back though because I believe a long-standing Prettier bug has been fixed (see discussion) that would format the above as:
<ClayIcon
className={{
'active': sortingMatch?.direction === 'asc',
'sorting-icon': true,
}}
/>There was a problem hiding this comment.
@FabioDiegoMastrorilli, thats fine, its not vital that you use the object notation, I just wanted to make sure you were aware of it.
...lib-clay/src/main/resources/META-INF/resources/data_set_display/views/table/TableHeadCell.js
Show resolved
Hide resolved
...lib-clay/src/main/resources/META-INF/resources/data_set_display/views/table/TableHeadCell.js
Outdated
Show resolved
Hide resolved
fabio-d-m
force-pushed
the
COMMERCE-5450
branch
2 times, most recently
from
7802864
to
318e39a
Compare
|
@wincent @bryceosterhaus @carloslancha @julien the PR has been updated! Thanks a lot for your code review! :D cc @jbalsas |
|
ci:forward |
|
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
✔️ ci:test:sf - 1 out of 1 jobs passed in 10 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: COMMERCE-5450 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#4067 |
This comment has been minimized.
This comment has been minimized.
|
All required test suite(s) passed. |
|
Pull request has been successfully forwarded to brianchandotcom#98054 |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#7354 |
Data Set Display should allow users to create new entities or editing existing ones directly from the component itself.
Requirements for editing:
Requirements for adding:
Example for item creation and editing (alwayOn true):
Example for item editing (alwayOn not true):
