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

Add option to clone custom widgets and its contents to another view #1689

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ArturoManzoli
Copy link
Contributor

  • Added a menu item on the Custom-widget-base to clone it to a selected view
Screenshare.-.2025-02-12.5_32_51.PM.mp4

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

Everything appears to be working fine and the code is good. We just need to change the return of that function and it's ready to go.

Comment on lines 873 to 882
const copyWidgetToView = (widget: Widget, viewName: string): boolean => {
let cloned = false
currentProfile.value.views.some((view) => {
if (view.name === viewName) {
const newWidget = JSON.parse(JSON.stringify(widget))
newWidget.hash = uuid4()
view.widgets.unshift(newWidget)
cloned = true
return true
}
return false
})
return cloned
}

Copy link
Member

Choose a reason for hiding this comment

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

Instead of returning true or false, as this is an "action" function, it should just "return void", this is, return nothing if everything goes right and throw an error in case something goes wrong.

Returning true or false in "action" functions is an old practice from languages like C that didn't have mecahisms to raise/throw errors.

@ES-Alexander
Copy link
Contributor

Could we have them linked by their name (per #1527), instead of cloning by default?

I think that would generally be more intuitive than needing to use a special menu on a widget, in a View you're not trying to change, to add something to a different View. Having an intuitive linking key would also allow keeping changes between them in sync automatically (which is likely usually desirable).

The interface for that could be similar to GitHub's branch filtering/naming dropdown, where an empty, unnamed Custom Widget Base would have a search bar for the name, with a label like "Find or create a Custom Widget..."1, then when you type a name it filters by existing ones, and if nothing matches exactly then there's a button to "Create a new Custom Widget called XX".

If we want to also support cloning then that could instead be performed when you rename one that already has mini-widgets in it.

Footnotes

  1. "Custom Widget" seems a little strange as a name for this, since it's functionally similar to the existing Mini-Widget Bar - I see the main differences as it being movable and collapsible, as well as having a name. Perhaps "Collapsible Container" would be more appropriate?

@ArturoManzoli ArturoManzoli force-pushed the Add-custom-widget-clone-to-view branch from 07c2ae9 to ed7902b Compare February 13, 2025 10:10
@ArturoManzoli
Copy link
Contributor Author

Could we have them linked by their name (per #1527), instead of cloning by default?

I think that would generally be more intuitive than needing to use a special menu on a widget, in a View you're not trying to change, to add something to a different View. Having an intuitive linking key would also allow keeping changes between them in sync automatically (which is likely usually desirable).

This is a very good feature to implement. I believe that having linked elements is indeed very handy, especially when custom-made components become more complex with detailed actions and configurations.

Since the input components are just getting started and users are beginning to adopt them, similar cloning results can be achieved using the feature implemented in this PR. Although re-cloning the component each time it is updated with complex configurations is needed, it's still easier than downloading the Widget's JSON, switching views, adding a Custom Widget Base, and then importing the JSON.

Would you mind creating an issue to implement the linking feature you described?

Regarding the “Custom Widget” name, I totally agree with you. Changing it to “collapsible container” would better describe its functionalities and the intention behind its creation. I will update the name in this PR.

[WidgetType.CustomWidgetBase]: true,
[WidgetType.CollapsibleContainer]: true,
Copy link
Member

Choose a reason for hiding this comment

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

This will cause the CustomWidgetBase widgets that people already have in their profiles to disappear.

We should perform a migration in this case.

I've added a migration.ts file in the repository some weeks ago where this can be fitted.

showSnackbar({
variant: 'error',
message: error.message || 'Error cloning widget.',
duration: 3000,
Copy link
Member

Choose a reason for hiding this comment

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

Since the default duration is 3000ms, this can be omitted and the method call will fit in a single line.

@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Feb 13, 2025
@ArturoManzoli ArturoManzoli force-pushed the Add-custom-widget-clone-to-view branch from 256868f to a72f6e7 Compare February 14, 2025 13:38
@ArturoManzoli ArturoManzoli force-pushed the Add-custom-widget-clone-to-view branch from a72f6e7 to e444941 Compare February 14, 2025 13:50
@ArturoManzoli ArturoManzoli marked this pull request as draft February 14, 2025 17:27
@ArturoManzoli ArturoManzoli force-pushed the Add-custom-widget-clone-to-view branch from e444941 to e78f000 Compare February 14, 2025 18:19
@ArturoManzoli ArturoManzoli marked this pull request as ready for review February 14, 2025 18:20
@ArturoManzoli
Copy link
Contributor Author

@rafaellehmkuhl Had some issues with the migration logic, but seems ok now.
Just check on a regular profile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-needed Change needs to be documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants