FEATURE: Implement Media Details Screen #148

grebaldi · 2022-11-05T12:42:22Z

resolves: #73

This is a full integration of the Media Details Screen. It's available with useNewMediaSelection set to true.

`Interaction`: Ad-Hoc `confirm`-, `alert`- and `prompt`-style dialogs

This PR introduces the Interaction concept to the @media-ui/core. It is a context-driven API that needs to be initialized for every application entry point. The idea is to enable ad-hoc confirm-, alert- and prompt-style dialogs without much React ceremony, but instead with an API closer to the browser-native confirm, alert and prompt functions.

The call-site looks as simple as this:

const SomeComponent = () => {
  const Interaction = useInteraction();

  const handleSomeAction = useCallback(
    async () => {
      if (await Interaction.confirm({ message: 'Do you really want to do this?' }) {
         await performSomeActionBecauseWeAreAllowedTo();
      }
    },
    [Interaction]
  );

  return (/* ... */);
};

Currently, only confirm has been implemented, because there's no need for the other ones in the scope of this PR (or perhaps the Media UI at large). In fact, this API (or one similar to this) should actually be provided by Neos.Neos.Ui.

`ApprovalAttainmentStrategy`: To ask or not to ask?

This lengthy name refers to another concept introduced by this PR. The problem is that depending on the context certain actions (like updating asset properties) should require confirmation by the user. For example: While editing asset properties in the media module is not a problem at all, editing asset properties in the secondary inspector might give an editor the impression that the changes will only be applied to the image variant that is used as a node property.

Media Module:

Peek.2022-11-16.15-48.mp4

Secondary Inspector:

Peek.2022-11-16.15-49.mp4

To solve this, different application entry points can now provide an ApprovalAttainmentStrategy. Depending on the action an ApprovalAttainmentStrategy can use the Interaction API to show a confirmation dialog or just return Promise.resolve(true) to automatically give the approval.

Components like <PropertyInspector /> then ask the currently applied ApprovalAttainmentStrategy for approval before they perform a critical action.

The ApprovalAttainmentStrategy covers the following actions:

updateAsset
setAssetCollections
setAssetTags
deleteAsset
replaceAsset

By default, only deleteAsset will ask for confirmation (This also solves: TODO: Use custom modal, see:

media-ui/Resources/Private/JavaScript/core/src/provider/MediaUiProvider.tsx

Line 74 in 1326bef

// TODO: Use custom modal

).

The application entry point in @media-ui/media-details-screen adds confirmation dialogs for all actions.

"Open in Media Ui" Action

Peek.2022-11-30.15-13.mp4

The Media Ui main application now reads the URL parameter searchTerm, which can be used as a means to initialize the search term input. Additionally, the search term can now have a special format /id:([0-9a-f]{8}-[0-9a-f]{4}-[0-5][0-9a-f]{3}-[089ab][0-9a-f]{3}-[0-9a-f]{12})/ in which case the search will look for an asset by its Id rather than performing a full text search. Also, the asset passed via the search term will be selected automatically.

With both these features in place, it is now possible to create a link into the Media Ui main application with a preselected asset. The <PreviewActions />-Component of the media details screen now provides such a link. The Media Ui main application will respectively open in a separate tab and show the asset from media details screen.

Along with this feature come unit test setups for both Server-Side and Client-Side code (mainly to test the SearchTerm-related domain logic). Regarding the JavaScript tests, I'd like to address the elephant in the room right away:

Why not `jest`?

I have chosen to use the good ol' mocha + chai combination for JavaScript unit tests. So, obviously, one might ask why not use jest (as probably the majority of contemporary projects do, including Neos.Ui)?

I recently have found jest to be a considerable maintenance burden with the node ecosystem moving towards native ESModule support. I have had projects halt the transition towards ESM entirely, because jest couldn't be made to work. ESM has been stable since Node v12 (see: https://nodejs.org/docs/latest-v12.x/api/esm.html), so for at least 3 years. TypeScript took a very long time to catch up to that, but since May of this year, full ESM support for Node has been added as well (see: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/). Unfortunately, ESM support in jest is still an experimental feature (see: https://jestjs.io/docs/ecmascript-modules), so it is likely to still be a reason for many packages in the ecosystem to hold back their transition towards ESM. I'm aware that this is not an immediate problem for the Media Ui, since it's based on CommonJS packages. But since I have fought this problem multiple times already, I've decided to not start any new testing setups on the basis of jest, but use mocha + chai instead, because both these packages are well maintained and just work.

mocha + chai do not come without their own problems though. Notably, I had to exclude @types/mocha from the setup, because those types were colliding with globals registered by testcafe (this is being addressed here: DevExpress/testcafe#6338). However, it is likely this issue would have come up with jest as well.

All of this being said, I wish to point out, that I'm of course open to discuss this point (This also applies to every other implementation decision I have made in this PR :)).

TODOs

The /id:([0-9a-f]{8}-[0-9a-f]{4}-[0-5][0-9a-f]{3}-[089ab][0-9a-f]{3}-[0-9a-f]{12})/ format is likely to be a false assumption, because assets from foreign asset sources may have Ids that are not UUIDs.
- This has been worked out by @Sebobo. By using the asset source neos to retrieve the local asset, Ids are guaranteed to be UUIDs.
None of this works with imported assets right now, because the image editor (Neos.Ui) only passes the local asset id rather than asst id + source id.
- Same here: Using the asset source neos solves this.

TODO

Sebobo · 2022-11-06T11:41:23Z

Hi @grebaldi, thx for jumping on this topic!
Did you see the results of our related discussion during the sprint in #146 ?

Especially with actions like replace, delete etc. we have to be careful in the UX as we need to make sure an editor understands the global consequences.

I wouldn't introduce a separate flag for this as we can replace all old features and I don't expect issues there.
And a separate feature package would be good.

grebaldi · 2022-11-08T19:12:26Z

Hi @Sebobo,

Did you see the results of our related discussion during the sprint in #146 ?

Thanks for the pointer, I wasn't aware of the discussion :)

I added the TODOs from there to the list in this PR. There's one question though:

Show Title, description & copyright in AssetEditor (see Shel.imageValidator) [...]

Does this refer to Shel.Neos.ImageChecker?

Regarding Architecture:

And a separate feature package would be good.

Okay, I'll add a media-details-screen package for this (it definitely feels better to move the details-specifics out of the media-module package).

I'd still like to clarify one thing in this context: AFAICT the Resources/Public/AssetEditor/Plugin.js target is being built from @media-ui/media-selection-screen. With the addition of @media-ui/media-details-screen, there are two ways to go about the plugin integration:

Establish two separate plugin builds:

@media-ui/media-selection-screen -> Resources/Public/MediaSelectionScreen/Plugin.js
@media-ui/media-details-screen -> Resources/Public/MediaDetailsScreen/Plugin.js

Introduce another package that integrates both plugins:

@media-ui/media-selection-screen <-(depends on)- @media-ui/neos-ui-plugin
@media-ui/media-details-screen <-(depends on)- @media-ui/neos-ui-plugin
@media-ui/neos-ui-plugin -> Resources/Public/AssetEditor/Plugin.js

I'd prefer the second approach. What do you think, @Sebobo?

Sebobo · 2022-11-10T08:28:30Z

Thanks for the pointer, I wasn't aware of the discussion :)

👍 no problem, should have linked the issue

Show Title, description & copyright in AssetEditor (see Shel.imageValidator) [...]

Does this refer to Shel.Neos.ImageChecker?

Yes, but it's isn't part of this issue. As the rewrite of the AssetEditor is separate (big) topic.

I'd prefer the second approach. What do you think, @Sebobo?

👍 yes that sounds perfect. It might be a bit confusing now that they all lie next to each other but have those not super obvious interdependencies. So at some point we might need to have 3 folders for packages, ui plugin, backend module and shared stuff.

grebaldi · 2022-11-16T15:06:02Z

I think there's a way to make replaceAsset work in the secondary inspector. But deleteAsset doesn't make much sense there, because one would get Exception #1462196420: Asset could not be deleted, because it is still in use. anyway - which means the asset simply cannot be deleted if it's available to the secondary inspector.

I'm going to experiment a little to see, whether replaceAsset can be done.

To hide deleteAsset (and potentially replaceAsset as well), I think it would be best to use the existing feature flag mechanism instead of adding more context. The application entry point in @media-ui/media-details-screen would then be able to override the global feature flag.

wdyt, @Sebobo?

Sebobo · 2022-11-16T15:13:17Z

deleteAsset makes no sense, I agree. So just get rid of it.
If we have a helpful dialog for replaceAsset I'm fine without a feature flag.

Sebobo · 2022-11-29T08:25:59Z

Hi @grebaldi I finally got the devserver and tests working again in #154
You should probably rebase even though the Details Screen is only in the ui plugin.

I also created a new issue #157 to start the process to also run e2e tests for the ui plugin.

grebaldi · 2022-11-30T15:08:39Z

Hi @Sebobo,

I've rebased and everything is green 🎉

I'm currently stuck with a problem that we should discuss probably: The image editor (Neos.Ui) passes an assetIdentity prop to the media details screen. For imported assets, this assetIdentity contains the local asset Id. This local Id cannot be resolved atm, so the details screen fails with imported assets.

I suppose it is possible to resolve the local asset Id, but I think we should discuss how to go about this.

Sebobo · 2022-12-02T12:39:37Z

I think for now we should disable the button then if we have an imported asset. There are several issues with imported assets atm and I have to workaround them in other places too.

So we should discuss this separately as the Media API needs to be fixed for that. But targeting this for Neos 9 would be essential I think.

Sebobo · 2022-12-02T12:51:07Z

Can't get your branch to work properly right now:

There are not console errors, so I have to dig deeper when I have more time.

grebaldi · 2022-12-02T13:20:42Z

@Sebobo

There are not console errors, so I have to dig deeper when I have more time.

Never mind, I'll take a look. Probably missed a bug with the recent changes, because I tested with imported assets last (and the screen you're seeing is the current behavior for imported assets - I apparently overlooked that regular assets are broken too).

I think for now we should disable the button then if we have an imported asset.

I believe that would require a change for the Image Editor (Neos.Ui). Regarding that: I've been thinking that it might be sensible to move that editor entirely over to Flowpack.Media.Ui (it definitely makes more sense to let it reside closer to the Media Context rather than the Core).

Such a migration would be out of scope for this PR though. So, I'm considering two options for going forward:

Either: Leave imported assets broken in the details screen for now, but merge (and ship) anyway if everything else works.
Or: Open up a long-running branch from master, move this PR over there and use the new branch to migrate the Image Editor, do imported assets fixes and so on. Finally, merge the long-running branch back into master once everything works.

Wdyt?

grebaldi · 2022-12-05T15:43:24Z

@Sebobo

@grebaldi: Never mind, I'll take a look. Probably missed a bug with the recent changes, because I tested with imported assets last (and the screen you're seeing is the current behavior for imported assets - I apparently overlooked that regular assets are broken too).

I investigated locally and regular assets seem to work fine for me. So, we'd need to take a look at what's happening in your setup.

Sebobo · 2022-12-06T15:25:48Z

Resources/Private/JavaScript/media-details-screen/src/MediaDetailsScreen.tsx

+                                                buildLinkToMediaUi={buildLinkToMediaUi}
+                                                assetIdentity={{
+                                                    assetId: this.props.imageIdentity,
+                                                    assetSourceId: '',


This is the problem. You need to define the asset source or it won't work when more than one is defined.
But I don't unterstand why it works then if you only have one asset source.

Suggested change

assetSourceId: '',

assetSourceId: 'neos',

Interestingly enough it still works with assets from other sources 🤷🏻‍♂️

Sebobo · 2022-12-06T15:34:40Z

Noticed some graphical things:

the screen pops while loading the asset due to the large placeholder. So we need a loading indicator here which is not so intense.
the inspector is not scrollable for me and not all information can be reached
switching to the variants tab breaks the layout height and the asset preview disappears
link styling in the usage dialog is incorrect (dark blue)
checkbox styling in replace dialog is broken (line break and missing icons)

grebaldi · 2022-12-19T10:59:16Z

Hi @Sebobo,

I was able to fix some of the issues you have found (and I'll continue with the rest of course). Along the way there are a couple of questions/remarks I'd like to clarify:

checkbox styling in replace dialog is broken (line break and missing icons)

I was able to fix the line break, but the missing icons seem to be an issue with Neos UI itself. In plugin context, the dialog uses the Checkboxes as provided by Neos UI. Those Checkboxes do not provide the greyed-out checkmark icon (maybe an older version did?).

the inspector is not scrollable for me and not all information can be reached

What browser did you use for testing? I was not able to reproduce this issue in Gecko, WebKit or Blink on Linux.

switching to the variants tab breaks the layout height and the asset preview disappears

How do I get to the variants tab? (Full disclosure: I don't know what the variants tab is 😅)

Sebobo · 2022-12-19T12:16:17Z

Thx!

The variant tab is enabled via the feature flag Neos.Neos.Ui.frontendConfiguration.Flowpack.Media.Ui.showVariantsEditor.

I'm testing with Brave. Will check your latest version later today and see if the error is still there and if I see why it's there if so.

Regarding the icon we then can check whether we need a hot fix in our code or fix it in the UI itself.

grebaldi · 2022-12-20T15:08:43Z

Looks like those icons have been explicitly removed: neos/neos-ui#2995

The media-module package currently requires:

"@neos-project/react-ui-components": "5.3.7"

The commit on Neos UI PR 2995 seems only to be included as early as 5.3.13:
neos/neos-ui@abceecc

That explains why the icon shows up in the backend module but not in Neos UI.

For a fix, I could update @neos-project/react-ui-components to the latest 5.3.*, but I seem to recall that there was an issue with updating the components, right?

Sebobo · 2022-12-20T18:39:41Z

ah ok 👌 Updating the components should be no problem. We have some other dependencies which break everything when being updated.

Sebobo · 2023-01-09T16:27:59Z

Looks good so far. My first tests looked fine!

I still have the issue with the content exceeding the content area and breaking the ui layout.
It might have something todo with the CSS Grid I introduced to the UI in 8.2.
Which Neos version are you using?

Sebobo · 2023-01-09T16:30:57Z

Classes/Infrastructure/Neos/Media/AssetProxyQueryIterator.php

+    private $assetProxyQuery;
+
+    /**
+     * @param AssetProxyQueryInterface $assetProxyQuery


You can remove all the duplicate type annotations in this class.

grebaldi · 2023-01-10T12:14:31Z

@Sebobo

I still have the issue with the content exceeding the content area and breaking the ui layout.
It might have something todo with the CSS Grid I introduced to the UI in 8.2.
Which Neos version are you using?

Yeah, this was it :) I've started development on 8.1 and didn't upgrade since. I can reproduce the issue now on 8.2 and will come up with fix soonish.

Update: I fixed the scrolling issue for 8.2 and also removed superfluous type comments from the PHP code.

This includes the following changes: - Package `@media-ui/media-details-screen` has been created - Code that is specific to the details screen has been moved from `@media-ui/media-module` to `@media-ui/media-details-screen` - Details.tsx - Preview.tsx - Package `@media-ui/neos-ui-plugin` has been created - Package `@media-ui/neos-ui-plugin` registers secondary inspector editors from both `@media-ui/media-details-screen` and `@media-ui/media-selection-screen` - Yarn script `watch:editor` has been renamed to `watch:plugin` - Yarn script `watch:plugin` has been adjusted to watch `@media-ui/neos-ui-plugin` instead of `@media-ui/media-selection-screen` - Yarn script `build:editor` has been renamed to `build:plugin` - Yarn script `build:plugin` has been adjusted to build `@media-ui/neos-ui-plugin` instead of `@media-ui/media-selection-screen`

…lsScreen This includes the following changes: - `InteractionProvider` has been introduced - Lives in `@media-ui/core` - A context-driven API that allows to open up a `confirm`-style Dialog from anywhere in the application - `faExclamationTriangle` was added to the icon library to replicate Neos.Neos.Ui Look&Feel - Same API can potentially be used to implement `prompt`- and/or `alert`-style Dialogs as well - Something like this should actually be provided by Neos.Neos.Ui - `ApprovalAttainmentStrategy` has been introduced - Lives in `@media-ui/core` - A strategy that allows different application entry points to define custom behavior when it comes to obtaining approval for certain actions - This allows the MediaDetailsScreen to ask for confirmation before e.g. deleting an asset, whereas the main media module does not - The default strategy is `AssumeApprovalWasGivenForEveryAction`, which never asks for confirmation - `InteractionProvider` gets instantiated in every application entry point - `ApprovalAttainmentStrategy` covers all critical actions - `updateAsset` - `setAssetCollections` - `setAssetTags` - `deleteAsset` - `replaceAsset` - Make sure that SelectBoxes for Tags and Asset Collections do not update prematurely (before approval/confirmation was given) - The underlying SelectBox component otherwise stays in a broken state if not synced back properly

This includes the following changes: - A PHPUnit setup was added to the project - PHPUnit ^9.5 - Unit tests can be found under `Tests/Unit` - This includes an additional job in the github actions `tests` workflow - The `SearchTerm` domain object has been introduced - A value object encapsulating newly introduced logic for search terms entered in the Media UI - A search term may now have a special format `/id:([0-9a-f]{8}-[0-9a-f]{4}-[0-5][0-9a-f]{3}-[089ab][0-9a-f]{3}-[0-9a-f]{12})/` in which case the search will look for an asset by its Id rather than performing a full text search - The `AssetProxyIteratorAggregate` domain object has been introduced - This has become necessary, because `QueryResolver` relies on processing iterables, which is fulfilled when dealing with an `AssetProxyQueryInterface` (or better: `AssetProxyQueryResultInterface`). However, looking up an asset by its Id via `AssetProxyRepositoryInterface::getAssetProxy` won't return an `AssetProxyQueryInterface`, but an `AssetProxyInterface`, which is not iterable. - There's two implementations of `AssetProxyIteratorAggregate`: - `AssetProxyListIterator` - will take 1 or more `AssetProxyInterface`s and provide the means to iterate over them - `AssetProxyQueryIterator` - will take an `AssetProxyQueryInterface` and resolves the query once iteration starts - The `createAssetProxyQuery` method of `QueryResolver` has been adjusted and is now called `createAssetProxyIterator` - Both, the `assetCount` and the `assets` method of `QueryResolver` have been adjusted accordingly

…ails screen This includes the following changes: - A JavaScript unit test setup was added to the project - mocha (^10.1) & chai (^4.3) - Unit tests are colocated with the respective item under test (in an `{itemName}.spec.ts` file each) - This includes an additional job in the github actions `tests` workflow - The `SearchTerm` domain object has been introduced to `@media-ui/core` - Replicates the funcationality of the PHP `SearchTerm` domain object - A `SearchTerm` can be created from the current window location (if it contains a `searchTerm` parameter) - Since `SearchTerm`s are now able to match an exact Id, it is possible to link to the Media Ui with a single asset being selected - A button "Open in Media Ui" has been added to the `<PreviewActions />`-Component in `@media-ui/media-details-screen` - If a `SearchTerm` refers to an exact Id, the asset source "neos" will be selected automatically, regardless of persisted selection - The `useSelectAssetSource` has been modified, so the returned `selectAssetSource` function accepts an asset source id rather than an entire `AssetSource` object

This includes the following changes: - The media details screen layout remains stable when switching from loading to loaded state - During loading state the preview image now has a low opacity to reduce flashing - Scrollability has been limited to the asset inspector area as opposed to the entire media details screen

Sebobo · 2023-01-12T08:25:09Z

Big ❤️, thx Wilhelm

Sebobo self-requested a review November 6, 2022 11:42

Sebobo added the Feature A new feature label Nov 6, 2022

Sebobo added this to the First stable release milestone Nov 6, 2022

grebaldi force-pushed the feature/73 branch from fd93db7 to a40bc10 Compare November 6, 2022 14:25

grebaldi force-pushed the feature/73 branch from eea8ef6 to 10419e5 Compare November 21, 2022 14:10

Sebobo mentioned this pull request Nov 29, 2022

Make it possible to run e2e tests for the UI plugin #157

Closed

grebaldi force-pushed the feature/73 branch 3 times, most recently from 87e88b3 to 52d3e8e Compare November 30, 2022 14:02

Sebobo reviewed Dec 6, 2022

View reviewed changes

grebaldi force-pushed the feature/73 branch 3 times, most recently from 261833a to 52dd187 Compare December 19, 2022 10:47

grebaldi force-pushed the feature/73 branch from 52dd187 to e75674b Compare December 20, 2022 14:42

grebaldi force-pushed the feature/73 branch from e75674b to 226e7e3 Compare December 21, 2022 14:25

grebaldi marked this pull request as ready for review December 22, 2022 09:44

grebaldi linked an issue Dec 22, 2022 that may be closed by this pull request

New secondary inspector #146

Closed

7 tasks

grebaldi requested a review from Sebobo January 5, 2023 09:06

Sebobo reviewed Jan 9, 2023

View reviewed changes

grebaldi added 9 commits January 10, 2023 13:29

FEATURE: Implement Media Details Screen

543663e

TASK: Implement custom preview actions for media details screen

25049aa

TASK: Update @neos-project/react-ui-components to 5.3.13

df0f5b9

TASK: Remove "Replace Asset" button from media details screen

33d5bc1

grebaldi force-pushed the feature/73 branch from 3f3a2b1 to 33d5bc1 Compare January 10, 2023 12:44

Sebobo approved these changes Jan 12, 2023

View reviewed changes

Sebobo merged commit a950c96 into Flowpack:master Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE: Implement Media Details Screen #148

FEATURE: Implement Media Details Screen #148

grebaldi commented Nov 5, 2022 •

edited

Loading

Sebobo commented Nov 6, 2022

grebaldi commented Nov 8, 2022

Sebobo commented Nov 10, 2022 •

edited

Loading

grebaldi commented Nov 16, 2022

Sebobo commented Nov 16, 2022

Sebobo commented Nov 29, 2022

grebaldi commented Nov 30, 2022

Sebobo commented Dec 2, 2022

Sebobo commented Dec 2, 2022

grebaldi commented Dec 2, 2022

grebaldi commented Dec 5, 2022

Sebobo Dec 6, 2022

Sebobo Dec 6, 2022

Sebobo commented Dec 6, 2022 •

edited

Loading

grebaldi commented Dec 19, 2022

Sebobo commented Dec 19, 2022

grebaldi commented Dec 20, 2022

Sebobo commented Dec 20, 2022

Sebobo commented Jan 9, 2023

Sebobo Jan 9, 2023

grebaldi commented Jan 10, 2023 •

edited

Loading

Sebobo commented Jan 12, 2023

FEATURE: Implement Media Details Screen #148

FEATURE: Implement Media Details Screen #148

Conversation

grebaldi commented Nov 5, 2022 • edited Loading

Interaction: Ad-Hoc confirm-, alert- and prompt-style dialogs

ApprovalAttainmentStrategy: To ask or not to ask?

"Open in Media Ui" Action

Why not jest?

TODOs

TODO

Sebobo commented Nov 6, 2022

grebaldi commented Nov 8, 2022

Sebobo commented Nov 10, 2022 • edited Loading

grebaldi commented Nov 16, 2022

Sebobo commented Nov 16, 2022

Sebobo commented Nov 29, 2022

grebaldi commented Nov 30, 2022

Sebobo commented Dec 2, 2022

Sebobo commented Dec 2, 2022

grebaldi commented Dec 2, 2022

grebaldi commented Dec 5, 2022

Sebobo Dec 6, 2022

Choose a reason for hiding this comment

Sebobo Dec 6, 2022

Choose a reason for hiding this comment

Sebobo commented Dec 6, 2022 • edited Loading

grebaldi commented Dec 19, 2022

Sebobo commented Dec 19, 2022

grebaldi commented Dec 20, 2022

Sebobo commented Dec 20, 2022

Sebobo commented Jan 9, 2023

Sebobo Jan 9, 2023

Choose a reason for hiding this comment

grebaldi commented Jan 10, 2023 • edited Loading

Sebobo commented Jan 12, 2023

grebaldi commented Nov 5, 2022 •

edited

Loading

`Interaction`: Ad-Hoc `confirm`-, `alert`- and `prompt`-style dialogs

`ApprovalAttainmentStrategy`: To ask or not to ask?

Why not `jest`?

Sebobo commented Nov 10, 2022 •

edited

Loading

Sebobo commented Dec 6, 2022 •

edited

Loading

grebaldi commented Jan 10, 2023 •

edited

Loading