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

Split AppNavigationItem into small components #470

Closed
7 tasks done
skjnldsv opened this issue Jul 9, 2019 · 10 comments · Fixed by #486
Closed
7 tasks done

Split AppNavigationItem into small components #470

skjnldsv opened this issue Jul 9, 2019 · 10 comments · Fixed by #486
Assignees
Labels
3. to review Waiting for reviews enhancement New feature or request feature: app-navigation Related to the app-navigation component

Comments

@skjnldsv
Copy link
Contributor

skjnldsv commented Jul 9, 2019

Same as #358 and after #25
#195
Refs: https://docs.nextcloud.com/server/16/developer_manual/design/navigation.html#app-navigation-menu

  • Entry with counter
  • Use Actions components
  • Collapsible component
  • Undo (with transition: slide)
  • Edit (with transition: fade)
  • Pinned item
  • Nested AppNavigationItem
<AppNavigationItem counter="test">
	This is the main entry
	<AppNavigationItem>Sub-element 1</AppNavigationItem>
	<AppNavigationItem>Sub-element 2</AppNavigationItem>
	<template #actions>
		<ActionButton @click="doSomething" />
	</template>
</AppNavigationItem>
@skjnldsv skjnldsv added feature: app-navigation Related to the app-navigation component enhancement New feature or request 1. to develop Accepted and waiting to be taken care of labels Jul 9, 2019
@marcoambrosini marcoambrosini self-assigned this Jul 10, 2019
@skjnldsv skjnldsv changed the title Split Navigation into small components Split AppNavigationItem into small components Jul 10, 2019
@marcoambrosini
Copy link
Contributor

marcoambrosini commented Jul 10, 2019

Hello everyone, Marco here! I'll be helping out for the next few months :)

I see 2 possible approaches in terms of organising the code. These two can also be mixed together, but I'm gonna describe here the two edge cases just to be clear:

#1 We could use all the and <AppNavigationWhatever /> and <Actions /> components directly in App.vue by nesting them into the <AppNavigationItem /> component. In this case we'd treat <AppNavigationItem /> almost like a wrapper:

<AppNavigationItem>
    This is the main entry
    <Avatar #avatar user="admin" display-name="Administrator" /> 
    <AppNavigationCounter #counter>123</AppNavigationCounter> 
    <template #actions>
        <ActionButton />
        <ActionButton />
        <ActionButton />
    <template>
</AppNavigationItem>

#2 We could make the <AppNavigationItem /> component a way smarter (kinda like it is now) and pass down everything as props, do the checks within the component and display the subcomponents (actions, avatar, counters etc) based on those checks. Something more like:

 <AppNavigationItem counter="123" avatar="http://xxxx.xxx"  actions="{ [ ... , ... , ... ] }" >

In this case the only components nested into <AppNavigationItem /> could be anoter <AppNavigationItem /> or `.

My personal preference is #1, I think that even if it might add more clutter at App.vue level, it would make things more readable at a glance. What do you think @skjnldsv @juliushaertl @georgehrke??

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Jul 10, 2019

Other examples:

<AppNavigationItem class="icon-xxx">This is the main entry</AppNavigationItem>
<!-- https://docs.nextcloud.com/server/16/developer_manual/design/navigation.html#entry-bullet -->
<AppNavigationItem bullet="0082c9">This is the main entry with a bullet</AppNavigationItem>
<AppNavigationItem class="icon-xxx">
    This is the main entry
    <AppNavigationCounter #counter :highlighted="true">123</AppNavigationCounter>
</AppNavigationItem>
<AppNavigationItem class="icon-xxx">
    This is the main entry
    <AppNavigationCounter #counter :highlighted="true">123</AppNavigationCounter>
	<AppNavigationItem>This is a sub-entry</AppNavigationItem>
	<AppNavigationItem>This is a sub-entry</AppNavigationItem>
	<AppNavigationItem>This is a sub-entry</AppNavigationItem>
</AppNavigationItem>
<AppNavigationItem class="icon-xxx" :collapsible="true">
    This is the main entry
	<AppNavigationItem>This is a sub-entry</AppNavigationItem>
	<AppNavigationItem>This is a sub-entry</AppNavigationItem>
	<AppNavigationItem>This is a sub-entry</AppNavigationItem>
	<template #actions>
        <ActionButton />
        <ActionButton />
        <ActionButton />
    <template>
</AppNavigationItem>

We still need to figure out the undo and edit mode.
@nextcloud/vue any ideas? Should we create a AppNavigationEdit element? Or a prop toggle :undo="true"? Or just let the dev strike the text and put a icon-history undo action? Then I guess we could only have to think about the edit mode?
https://docs.nextcloud.com/server/16/developer_manual/design/navigation.html#edit-entry

@korelstar
Copy link
Contributor

When looking at the discussion in #447, I suggest to use a separate template slot for children items. Otherwise it will be hard to distinct between children and main elements.

Regarding undo, I prefer a prop toggle.

@skjnldsv
Copy link
Contributor Author

Regarding undo, I prefer a prop toggle.

Any specific reasons? :)

Otherwise it will be hard to distinct between children and main elements.

What elements do yuo have in mind?

@marcoambrosini
Copy link
Contributor

I agree with @korelstar about the undo, I think that anything that would allow to change AppNavigationItem, It's behaviour (i.e. always open children), and it's title (i.e. edit) should be a prop. As per the nested components I don't think that there's the need to use templates, with proper naming and indentation it should look pretty clear right?

@korelstar
Copy link
Contributor

Regarding undo, I prefer a prop toggle.

Any specific reasons? :)

It's because I think that the component should provide a semantic for undo. The design functionality should be capsulated in the component. This would make design changes (e.g. animations) easier and streamlined through different apps.


Otherwise it will be hard to distinct between children and main elements.

What elements do yuo have in mind?

Nothing concrete, but in the context of #420, #445 and #447 I learned, that it is better to make things explicit. You can't check for a child component by name, because someone could use it's own component which is based on AppNavigationItem (I'm doing this in notes). And you should not assume that all child components are of the expected type, because someone could add a child element for another purpose (e.g. colorize a single word).

@skjnldsv
Copy link
Contributor Author

You can't check for a child component by name, because someone could use it's own component which is based on AppNavigationItem

Well, if you have to use your own custom component, it means our current one is not good enough and needs to be perfected :)
This is where we have the balance between a standardized components set and devs doing whatever they want!

@korelstar
Copy link
Contributor

Well, if you have to use your own custom component, it means our current one is not good enough and needs to be perfected :)

This doesn't apply in my case. While AppNavigationItem is a generic component, I want to use a subject-specific component (NavigationNoteItem, see link above) which uses the generic component but implements semantic for the specific domain. This can't be integrated in the generic component.

In other languages, I would use inheritance for that (NavigationNoteItem would be a subclass of AppNavigationItem), but this is not possible with Vue components.

@skjnldsv
Copy link
Contributor Author

Ah yes!
We should indeed be able to use renderless components !

@marcoambrosini
Copy link
Contributor

@skjnldsv what should I do with the CSS? I was starting to move it from the server to the Vue components, but in the classes there are many references to mixins and variables that live on the server. I'm not able to compile the code because the compiler looks for those references in the nextcloud-vue folder and doesn't find them.
In theory, once you're developing the apps using the npm package, these componends should have access to those mixins and variables right?

@marcoambrosini marcoambrosini added 3. to review Waiting for reviews and removed 1. to develop Accepted and waiting to be taken care of labels Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: app-navigation Related to the app-navigation component
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants