Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: Layout changes for creation dialog #82

Merged

Conversation

grebaldi
Copy link
Member

@grebaldi grebaldi commented Jul 17, 2021

This PR implements adjustments in order to integrate the new Media UI seamlessly as a secondary editor within the NodeCreationDialog (implemented in neos/neos-ui#2870).

The changes made consist of slight CSS changes as well as some fixes for compatibility issues:

Make MediaUiProvider aware of NodeCreationDialog

First of all, the MediaUiProvider needed the information that it is being used within the NodeCreationDialog. The condition to determine this however is a little flimsy right now, due to current limitations of the Main UI.

This PR is checking, whether the NodeCreationDialog is open, because we can assume that if componentDidMount of
MediaSelectionScreen is being called while the dialog is open, the component definitely must be rendered within the dialog, since the dialog is modal and thus prevents all other interaction from happening.

We might have more than 1 instance of MediaSelectionScreen at once - one in a SecondaryInspector screen and
one in the NodeCreationDialog. With the solution in this PR, these two do not interfere (verified by manual testing). This only works because the condition happens to have the implication described above.

Furthermore, this PR prevents the left side bar of the Neos UI from closing, when the MediaSelectionScreen is being rendered within the NodeCreationDialog. Also, the padding around the MediaSelectionScreen is removed, since the NodeCreationDialog already provides sufficient padding.

Notes for the future

  • The Neos.Ui should provide a more official way to let Editor-components know, whether they are being rendered inside the NodeCreationDialog

Adjust BottomBar for NodeCreationDialog

Before After
BottomBar-before BottomBar-after

The BottomBar is positioned in a way that makes it look as if it is part of the dialog.

The grid layout is adjusted, so that each column takes up as much space as is needed, as opposed to the left-most and right-most columns reserving 350px worth of space.

Adjust ListView for NodeCreationDialog

Before After
Listview-before ListView-after

The height of the ListView is adjusted, so that it doesn't overflow the height of the reserved space for secondary editors. Doing so prevents a second scrollbar from appearing when users switch to the ListView.

Adjust App Layout to NodeCreationDialog

Before After
App-before App-after

The height of the App Layout is calculated in a way that it compensates for the bottom bar and the padding provided by the NodeCreationDialog for secondary editors.

Remove default left padding from Pagination list

To adjust the Pagination list better to a more narrow bottom bar, I removed the default left padding of the <ol>:

padding

I assumed, that the padding wasn't intentional to begin with, so I put no condition to that. The padding is gone for all cases.

Prevent any media ui action from closing the NodeCreationDialog accidentally

This one's a bit tricky. Basically, Neos.Ui can't actually stack dialogs on top of each other. It happens to work optically, but due to the click-outside handling of the Neos.Ui Dialog Component, each dialog stacked on top of another one will close immediately when interacted with.

I opened an issue in Neos.Ui with some more info: neos/neos-ui#2925

So, the workaround I implemented to address this is probably a little questionable. It adds a custom decorator component for the Neos UI Dialog that sets the attribute data-ignore_click_outside="true" on the dialog container, so that it is excluded from click-outside handling. Clicking on the second overlay still closes both dialogs though.

This solution is crude and relies on a deprecated ReactDOM-API (ReactDOM.findDOMNode to be exact - I had to put a eslint-disable-line at that as well...). But for the time being, it'll at least make the inner dialogs usable in most cases.

Notes for the future

Make AssetPreview compatible with NodeCreationDialog

This addresses two problems:

  1. The styles for the Lightbox component were missing from the Plugin.css bundle. I solved this by including the main bundle for the Neos.Ui as well.
  2. The Lightbox was constrained to the area of the NodeCreationDialog reserved for secondary editors. I solved this by creating a separate Lightbox container on-demand when the Lightbox is rendered inside the NodeCreationDialog.

Notes for the Future

  • The Lightbox styles are still bundled inside the Plugin.css, but with hashed class names (CSS modules). To remove them the need to be excluded from the build somehow and this will likely require a change in Neos.Ui.
  • Including the main bundle for the Main UI also introduces a lot of redundancy. There might be a way to split out a separate CSS bundle for The Lightbox only using Parcel. Another way might be to leave them out of the bundling process and just copy them from node_modules to Public. Not sure what's the best choice of action here 😅

closes: #68

@grebaldi grebaldi marked this pull request as draft July 17, 2021 18:17
So, the condition to determine whether the selection screen is currently
being shown inside the NodeCreationDialog is a little flimsy right
now...

This commit is just checking, whether the NodeCreationDialog is open,
because we can assume that if `componentDidMount` of
`MediaSelectionScreen` is being called while the dialog is open, the
component definitely must be rendered within the dialog, since the
dialog is modal and thus prevents all other interaction from happening.

Using this solution, we might have more than 1 instance of
`MediaSelectionScreen` at once - one in a SecondaryInspector screen and
one in the NodeCreationDialog. These two do not interfere (verified by
manual testing). This only works because the condition happens to have
the implication described above.

This commmit also prevents the left side bar of the Neos UI from
closing, when the `MediaSelectionScreen` is being rendered within the
NodeCreationDialog.

Additionally, the padding around the `MediaSelectionScreen` is removed,
since the NodeCreationDialog already provides sufficient padding.
@grebaldi grebaldi force-pushed the bugfix/68/layoutChangesForCreationDialog branch from 4a8f5b9 to 026b6f6 Compare July 18, 2021 16:27
This changes the grid layout for the BottomBar as well as its
positioning when rendered in the NodeCreationDialog.

The BottomBar is positioned in a way that makes it look as if it is part
of the dialog.

The grid layout is adjusted, so that each column takes up as much space
as is needed, as opposed to the left-most and right-most columns
reserving 350px worth of space.
This adjusts the height of the ListView if rendered inside the
NodeCreationDialog, so that it doesn't overflow the height of the
reserved space for secondary editors.

Doing so prevents a second scrollbar from appearing when users switch to
the ListView.
The App Layout gets a different height when being rendered inside the
NodeCreationDialog.

The height is calculated in a way that it compensates for the padding
provided by the NodeCreationDialog for secondary editors.
This removes the left padding from the Pagination list which made it
appear slightly off-center.
This commit creates a (partial) workaround for the issue described in
neos/neos-ui#2925.

It adds a custom decorator component for the Neos UI Dialog that sets
the attribute `data-ignore_click_outside="true"` on the dialog
container.

This solution is crude and relies on a deprecated ReactDOM-API. But for
the time being, it'll at least make the inner dialogs usable in most
cases.

Once neos/neos-ui#2925 is fixed, this solution
should be removed.
This commit addresses two problems:

1. The styles for the Lightbox component were missing from the
Plugin.css bundle. This was solved by including the main bundle for the
Neos.Ui as well.
2. The Lightbox was constrained to the area of the NodeCreationDialog
reserved for secondary editors. This was solved by creating a separate
Lightbox container on-demand when the Lightbox is rendered inside the
NodeCreationDialog.
@grebaldi grebaldi force-pushed the bugfix/68/layoutChangesForCreationDialog branch from 026b6f6 to 07d93d7 Compare July 18, 2021 18:18
@grebaldi grebaldi requested review from Sebobo and dimaip July 18, 2021 18:49
@grebaldi grebaldi changed the title WIP: Layout changes for creation dialog Layout changes for creation dialog Jul 18, 2021
@grebaldi grebaldi marked this pull request as ready for review July 18, 2021 18:51
@grebaldi grebaldi changed the title Layout changes for creation dialog BUGFIX: Layout changes for creation dialog Jul 18, 2021
@grebaldi
Copy link
Member Author

The codestyle and e2e checks aren't necessarily failing due to this PR, since they are failing on master as well (with the same output).

@Sebobo Sebobo merged commit fe4eee0 into Flowpack:master Jul 29, 2021
@Sebobo
Copy link
Member

Sebobo commented Jul 29, 2021

Awesome, works and looks really nice :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle layout change when shown in creation dialog
2 participants