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

custom actions for navigation item's primary icon #246

Closed
wants to merge 1 commit into from

Conversation

korelstar
Copy link
Contributor

For the notes app, @jancborchardt suggested to put the star (favorite) on the left side of the navigation item (please see nextcloud/notes#2). This is no problem to realize, because I can add an icon to a navigation item.

However, I would like to connect this icon with an action (toggle favorite) that differs from the main action behind the item's label (open note). Therefore I would like to extend the component AppNavigationItem.

I found the <img> inside the AppNavigationItem and extended it with some more properties in order to be suitable for my purposes. However, there may be other possibilities to realize this, e.g. extending the <button> which is used for the collapse icon, now (see https://github.com/nextcloud/nextcloud-vue/blob/icon-action/src/components/AppNavigationItem/AppNavigationItem.vue#L34). A disadvantage of using the button is that we need some more CSS in order to work. However, my current solution using the existing <img> has a quite small click area.

If you want to see how I'm using this, please have a look on nextcloud/notes#290 and especially

https://github.com/nextcloud/notes/blob/1a84b2487c4a03fb1382614a16208db2eef9917e/src/App.vue#L200-L202

and

https://github.com/nextcloud/notes/blob/1a84b2487c4a03fb1382614a16208db2eef9917e/css/app-navigation.scss#L27-L45

Another improvement would be to group the item.icon* (e.g. item.iconAction) properties to item.icon.* (e.g. item.icon.action).

What do you think?

@korelstar korelstar added 2. developing Work in progress feature: app-navigation Related to the app-navigation component design Design, UX, interface and interaction design labels Feb 10, 2019
@korelstar korelstar mentioned this pull request Feb 11, 2019
54 tasks
@skjnldsv
Copy link
Contributor

However, I would like to connect this icon with an action (toggle favorite) that differs from the main action behind the item's label (open note). Therefore I would like to extend the component AppNavigationItem.

I'm not sure about this. I thinks this is not really the place for that. This is difficult to discover :)
@jancborchardt any opinions?

@jancborchardt
Copy link
Contributor

How about the star icon just being a representation, but not the action? Favorite notes would have the star on the left, but the action itself would be inside the 3-dot menu as "Add to favorites"/"Remove from favorites" just like in files.

This seems to be better and working with the standard than nextcloud/notes#2

@korelstar
Copy link
Contributor Author

Hmm. I'm not sure if this a good idea. In the files app, it's a different situation: the icon is primarily a preview and the star is comparatively small. There, it's clear, that the star has no action. In the notes app, the star would fill the whole icon and users would expect that icon to have an action, just like in the Android Nextcloud Notes app.

Another drawback: the nextcloud-vue library provides the three-dot menu if there are more than two actions, only.

@jancborchardt
Copy link
Contributor

Another drawback: the nextcloud-vue library provides the three-dot menu if there are more than two actions, only.

Right, I guess we need to add a "Sharing" action at some point anyway. ;)

@korelstar
Copy link
Contributor Author

Okay, I think I kind of accepting the principle of not allowing a custom action for the icon. Hence, I will close this.

But before doing this, I would like to point to a change proposal that I made in this PR implicitly: Before, there were two cases in the code for displaying the main text with link: using a href and using a JavaScript action. For this, there were some code duplicated. In this PR, I realized an approach in order to combine both scenarios into one code snippet (using a helper function). Should I make a new PR for this part or do you are not interested in this?

@jancborchardt
Copy link
Contributor

@korelstar our JS pros @skjnldsv @juliushaertl will have to comment on that. :)

@skjnldsv
Copy link
Contributor

@korelstar are you talking about creating a sub component ?

@korelstar
Copy link
Contributor Author

No, just like this PR (please have a look at https://github.com/nextcloud/nextcloud-vue/pull/246/files) but without the unwanted iconAction (and iconTitle and iconClass) stuff.

@skjnldsv
Copy link
Contributor

@korelstar Yes, make sense! :)
Go for it, all optimisation or duplicate code optimisation are welcome 🤗

@korelstar
Copy link
Contributor Author

@skjnldsv see #307 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress design Design, UX, interface and interaction design feature: app-navigation Related to the app-navigation component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants