🇮🇳India @soaratul

soaratul → created an issue.

I guess make text colour red for those that are not in the current list of redux store, but what about for the new items(I mean those are not in the redux store but are there in conflicts as additional item).

BTW it's very rare now we will get conflicts as clicking on Review N changes button now we are refetching pending changes also on every 10 seconds we are fetching.

What exact change we need to do in the UI for 👇?
When conflict IDs exist, these will be marked up differently in the pending changes UI

soaratul → created an issue.

I am encountering an issue where CSS variables used in Radix-UI theme components are not being recognized while rendering it in storybook's story. For example, in components with the following CSS:

padding-left: var(--space-3);
padding-right: var(--space-3);
color: var(--accent-contrast);

When rendering the component in Storybook and inspecting the element, these variables have no value assigned. Clicking on them shows an error like --space-3 is not defined or --accent-contrast is not defined.

It seems that the required CSS variables are either not being loaded or not defined in the context of Storybook. debugging it I believe I get the root cause of this soon.

soaratul → created an issue.

@wim-leers Added gif for stories!

Below google sheet viewable for everyone having the list of all components used in XB that are not connected to Redux store and if we can write stories as of not or not with reason.
https://docs.google.com/spreadsheets/d/1h61Yo__ejYYSQrxHMboiEu7Tw5DS5tyf...

@finnsky Thanks for your suggestion it looks great, but we’re implementing Storybook for the XB React app’s UI, and not for SDCs.

soaratul → created an issue.

I think it's now in a good shape to be reviewed.

soaratul → made their first commit to this issue’s fork.

I tried setting pointer-events to none on the top bar and zoom controls while isDragging is true but it did not worked, because actually the scroll starts only when the mouse is approximately 10px close to browser edge(top/bottom) and the top bar and zoom controls are far away.

I found that top bar and zoom controls actually not causing the misbehave of auto scroll and it's the default browser behaviour(by default auto scroll starts when mouse is very close to edge while dragging), I also found scrollSensitivity and scrollSpeed option used in ui/src/hooks/usePreviewSortable.ts but seems there is no effect of these options and only default browser behaviour is working because these both options works only if forceFallback is set as true, and setting forceFallback as true stops the drag and drop features completely(I didn't investigated further though).

I tried allow-same-origin and forceFallback, as mentioned here in this sortablejs issue thread here but none worked 😞

soaratul → made their first commit to this issue’s fork.

Assigning to myself to look into the issue.

I am concerned for a page having too many components, in that case all the component's props data would get stored into redux cache after fetching them - so for too many components there would be too many api calls on page load itself, that might slow down the performance of the page.

@wim-leers, I don't think this is related to #3467954

I also debugged and found that in case of prop value is an object, we are not updating the preview and seems it will require some backend work.

@bnjmnm I debugged and found that the problem is with Panel(ui/src/components/Panel.tsx) > Topbar(ui/src/components/topbar/Topbar.tsx) component.

To solve this, there are two approaches I found

1. Remove asChild prop from Topbar > Menubar.Root component
2. Wrap the Panel component with forwardRef

The 1st approach is easy and quick, but it will not produce the same html structure as it was supposed to be, 2nd approach works with no error and with asChild prop as well.

Note: Both approaches pass all test cases

Changed steps to reproduce in issue summary

soaratul → created an issue.

What we have done assuming to remove test case mentioned by @balintbrews in the current MR is working on my local environment but the same is not working/passing the test on GitHub - so in short we cannot move ahead with this approach.

I tried to assert contextualMenuRect.left = clickEvent.pageX + iframeRect.left but this calculation is not working - note that clickEvent.pageX is returning left position relative to the iframe where the mouse was clicked so adding iframeRect.left + clickEvent.pageX, that should be equal to contextualMenuRect.left(as contextual menu is being appended to main viewport). I did same for top position as well using contextualMenuRect.top = clickEvent.pageY + iframeRect.top but no luck.

Apart from above I tried few more ways around it but no luck.

MR comment addressed along with what we discussed, please review.

soaratul → made their first commit to this issue’s fork.

soaratul → made their first commit to this issue’s fork.

@jessebaker!

There seems to a bug that means that pinch-zooming on a trackpad on Mac can also sometimes zoom the whole browser, this should also be addressed here.

I know for this ☝️ we are about to create an separate issue still I tried to fix this using touch-action: none; and other JS/React ways but no luck.

Apart from this ☝️ I tried to write a test case 👇 in cypress to validate default browser behaviour on zoom in/out but again found nothing related to this in cypress.

This seems to be fixed but the new test introduced in this MR is not actually testing that. TBC if it's even possible to test that the default browser behaviour is being prevented in a Cypress test!

For this 👇 I cannot check as my external keyboard having numeric pad is not working with my Mac by the way are the other keys plus/minus(+/-) for zoom in/out using numeric pad are passing the test?

If you focus in the iframe and then press (specifically) NUMPAD0 it doesn't pass that event up to the parent. (The other 0 key works)

@wim-leers Yes exactly
- #7 has been done with assumption
- We can now move to the subsequent in #5, along with what I've mentioned in #15

Also: I will be resolving the conflicts while verifying the test cases - and test cases can be verified only after completing all the task mentioned in #15 e.g. stop dragging, selecting, rearranging, deleting etc.. while editing component(empty required field value).

I wrote all test cases suggested by @jessebaker and team based on assumption only - like we will not allow to...

1. Select other component if one is being edited and it's required field is empty
2. Drag other component
3. Rearrange components
4. Delete a component
5. etc..

I hope we would be doing all these checks and implementation from FE side once after fixing the BE issue to pass all the test cases we wrote with assumption.

I have written all test cases based on assumption only so might be possible that it may require some changes based on actual implementation.

soaratul → made their first commit to this issue’s fork.

Changing status to Needs work to write test case for zoom reset and to fix a bug that I found while writing test case.

soaratul → made their first commit to this issue’s fork.