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

feat: generic dynamic context menu + reel gallery integration #3071

Merged
merged 45 commits into from
Jan 20, 2025

Conversation

lorenzo-ranciaffi
Copy link
Contributor

@lorenzo-ranciaffi lorenzo-ranciaffi commented Jan 7, 2025

What does this PR change?

Introduce a generic context menu system that is completely configurable and it uses the MVC structure.

Its definition is done via code using the class GenericContextMenu. As of now, the available components are:

  1. Button with text and icon ButtonContextMenuControlSettings
  2. Toggle with text ToggleContextMenuControlSettings
  3. Graphical separator SeparatorContextMenuControlSettings (a divider composed by a simple grey line)

Every control has its specifics parameters in order to adjust its appearance and specify the action to execute on interaction.
All the appearance configurations have already a default value so that they can be used out of the box.

The menu can be opened using the MVC Manager mvcManager.ShowAsync(GenericContextMenuController.IssueCommand(new GenericContextMenuParameter()) by providing

  1. a reference to the main configuration class
  2. a Vector2 with the anchor position of the context menu

Optionally there are other parameters to provide:

  1. a Rect which defines the usable space for the context menu (default is the full viewport) which is used to calculate the final position avoiding overlaps with the rect itself
  2. an action that is triggered as context menu is shown
  3. an action that is triggered as the context menu is closed
  4. a task that on completion triggers the context menu to close
  5. a dictionary with initial values for the controls (at the moment is used only by the toggle control to dictate the initial state)

An integration in the camera reel gallery is included in this PR as a working example.

How to test the changes?

  1. Launch the explorer
  2. Open the camera reel gallery
  3. Click on the 3 dots button of a thumbnail and open the context menu
  4. Verify that the context menu itself works and that also every function works as expected (keep in mind that the download reel function may result in an error at the time of testing)

Our Code Review Standards

https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md

@lorenzo-ranciaffi lorenzo-ranciaffi self-assigned this Jan 7, 2025
# Conflicts:
#	Explorer/Assets/DCL/InWorldCamera/CameraReelGallery/CameraReelGallery.asmdef
#	Explorer/Assets/DCL/PluginSystem/DCL.Plugins.asmdef
#	Explorer/Assets/DCL/PluginSystem/Global/Global Plugins Settings.asset
#	Explorer/Assets/Scripts/Global/Dynamic/DynamicWorldContainer.cs
@lorenzo-ranciaffi lorenzo-ranciaffi marked this pull request as ready for review January 8, 2025 12:32
lorenzo-ranciaffi and others added 4 commits January 9, 2025 09:42
# Conflicts:
#	Explorer/Assets/DCL/ExplorePanel/Assets/ExplorePanelUI.prefab
#	Explorer/Assets/DCL/InWorldCamera/CameraReelGallery/Assets/CameraReel.prefab
Copy link
Collaborator

@lorux0 lorux0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! Overall it is good enough for our current needs, but i dont 100% agree with the approach of scriptable objects and how the context menu is configured. First of all i dont see the real benefit of creating scriptable objects instead of having the pre-defined prefabs with a simply vertical layout that aligns all of its internal components. I see three downsides:

  1. We will not be able to see how the context menu looks until we run the app and finally see it in runtime. So if we need to do any modifications we will have to edit the SO and re rerun the app to check if changes are good enough, which is something that i do not find efficient.
  2. We cannot create context menues from code at runtime either, since we always need an SO with the settings behind it. So if we need to create something dynamic we will have limitations.
  3. We are not following the unity standard for UI components, which on all of them you can either configure it from the inspector in the scene and/or create a prefab from it.

Anyway i think that these concerns are not blocking issues at the moment, but we will see how it evolves during different use cases.

# Conflicts:
#	Explorer/Assets/DCL/PluginSystem/Global/Global Plugins Settings.asset
# Conflicts:
#	Explorer/Assets/AddressableAssetsData/AssetGroups/Essentials.asset
#	Explorer/Assets/DCL/ExplorePanel/Assets/ExplorePanelUI.prefab
#	Explorer/Assets/DCL/PluginSystem/Global/ExplorePanelPlugin.cs
#	Explorer/Assets/DCL/PluginSystem/Global/Global Plugins Settings.asset
Copy link
Collaborator

@mikhail-dcl mikhail-dcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good apart from the lack of typed bindings between views and configuration.

I left the corresponding comments, let's discuss and find a proper solution

# Conflicts:
#	Explorer/Assets/Scripts/Global/Dynamic/DynamicWorldContainer.cs
@lorenzo-ranciaffi lorenzo-ranciaffi requested review from a team as code owners January 16, 2025 08:24
Copy link
Collaborator

@mikhail-dcl mikhail-dcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my doubts 💪

Copy link

@Ludmilafantaniella Ludmilafantaniella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Feat review verified on Windows and Mac. 3 dots button of a thumbnail and open the context menu.
Context menu itself works and that also every function works as expected. Smoke test performed covering:

  • Login
  • Backpack
  • Teleport between scenes and worlds (The Inn, Metadynelabs, Doll House)
  • Emoting
  • Social interactions.

@lorenzo-ranciaffi lorenzo-ranciaffi merged commit 05803bd into dev Jan 20, 2025
10 of 13 checks passed
@lorenzo-ranciaffi lorenzo-ranciaffi deleted the feat/dynamic-context-menu branch January 20, 2025 17:22
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.

5 participants