COMMERCE-5939 Dataset Fixes + minor updates #865
Conversation
|
CI is automatically triggering the following test suites:
|
liferay-continuous-integration
added
ci:test:sf - pending
ci:test:sf - failure
and removed
ci:test:sf - pending
labels
❌ ci:test:sf - 0 out of 1 jobs passed in 3 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: COMMERCE-5949 1 Failed Jobs:For more details click here.
|
|
Jenkins Build:test-portal-source-format#152 |
|
ci:test:sf |
✔️ ci:test:sf - 1 out of 1 jobs passed in 4 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: COMMERCE-5949 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#5334 |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#6497 |
jbalsas
changed the title
|
Hey @FabioDiegoMastrorilli, I'm having a hard time wrapping my head around this. Is there an easy way to test the issue? The LPS only mentions a "hostile" environment, but how do we go and reproduce it? Since this is mostly removals in @wincent, do you want to maybe just do a quick 30k feet review in case something triggers your spidey-sense? |
jbalsas
changed the title
| const [datasetDisplaySupportSidePanelId] = useState( | ||
| props.sidePanelId || 'support-side-panel-' + getRandomId() | ||
| ); | ||
| const [datasetDisplaySupportSidePanelId] = useState(props.sidePanelId); |
There was a problem hiding this comment.
It is a little odd that you would use useState for something that you never change. If you just want to associate a value once with a component and never update it, useRef is probably the right tool for that (in fact, IIRC, if you look at the implementation of useState in React, you'll see that it is using refs under the covers to associate data with components).
There was a problem hiding this comment.
It totally makes sense and I'll bear it in mind for the next PRs but I wouldn't change now. This dataset is the old one which is going to be deleted soon (after the minicart component usage switches to the new one)
| trigger.addEventListener('click', (e) => { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
Best not to abbreviate (ie. e → event).
There was a problem hiding this comment.
Omg, I'm really terrible with this rule. Sorry guys
There was a problem hiding this comment.
I wouldn't worry too much about this. We can actually lint for the common cases (eg. e, evt and friends) and then you won't have to worry about it. I thought we might have an issue for that already but I couldn't find it, so I created this one.
| Liferay.on(OPEN_MODAL_FROM_IFRAME, (payload) => { | ||
| let firstAvailableModalId = null; | ||
|
|
||
| // eslint-disable-next-line no-for-of-loops/no-for-of-loops |
There was a problem hiding this comment.
Good news is that we're turning this rule off in the next release of @liferay/eslint-config so we'll be able to delete this suppression soon.
| return `${iframeHandlerModalNamespace}${counter++}`; | ||
| export function subscribeModal(modalNode) { | ||
| const id = `${iframeHandlerModalNamespace}${modalsCounter++}`; | ||
| registeredModals.set(id, modalNode); |
There was a problem hiding this comment.
Brian would usually ask for a blank line above this.
|
Hi Chema, this ticked explains the one of the possible scenarios where it breaks. |
Thanks @FabioDiegoMastrorilli! I feel that's a much better ticket to solve than COMMERCE-5949 since it has all the information... I'd recommend trying to prioritize the more detailed reports over the others. Will make it easier for everyone to track down why things are the way they are :) If you're going to make any changes, would you mind renaming the commits to point to COMMERCE-5939 instead and close the other one as duplicate? |
|
Of course! no problem! |
fabio-d-m
force-pushed
the
COMMERCE-5949
branch
from
6dadcfc to
4f0580c
Compare
fabio-d-m
changed the title
|
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 6 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: COMMERCE-5949 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#2816 |
|
All required test suite(s) passed. |
|
Pull request has been successfully forwarded to brianchandotcom#99760 |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#631 |
This PR concerns dataset displays, iframes and the way they communicate.
COMMERCE-5939
when a dataset got rendered within an "ostile" container with a
display: nonecss property,cellRef.current.getClientRects()returned an empty array causing this code to break the component.I used an IntersectionObserver to avoid the problem. The callback gets invoked only the first time the component gets rendered and, eventually (if it is hidden), when it becomes visible, then it disconnects the Observer.
COMMERCE-5968
In order to let customers using Java extension points and allow them to render content using JSPs, we massively deal with iframes. Wherever we want some content to be rendered within a side panel we wrap it within a
side-panel-contenttaglib.I did some refactoring and I removed the old side panel implementation we had in commerce module since it's now part of the dataset in
frontend-taglib-clay.