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

Implement sidebar component #340

Merged
merged 3 commits into from
Apr 15, 2019
Merged

Implement sidebar component #340

merged 3 commits into from
Apr 15, 2019

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Apr 8, 2019

@nextcloud/vue

Closes nextcloud/server#10289

Standardisation is minimal but looks very nice and allow quite a few specificity that looks really great!

1 2
dev skjnldsv com_apps_vueexample_ dev skjnldsv com_apps_vueexample_ (2)
dev skjnldsv com_apps_vueexample_ (3)
<app-sidebar v-show="show"
	title="andy-yeo-1387151-unsplash.jpg"
	subtitle="4,3 MB, last edited 19 days ago"
	background="/core/preview?fileId=54&x=1880&y=993&a=true"
	:actions="menu"
	:tabs="tabs"
	@close="closeSidebar">
	<template #primary-action>
		<button class="primary">Leave call</button>
	</template>
	<template #secondary-action>
		<input name="link-checkbox" id="link-checkbox" class="checkbox link-checkbox" value="1" type="checkbox">
		<label for="link-checkbox" class="link-checkbox-label">Share link</label>
	</template>
	<tab name="Chat" icon="icon-talk">
		this is the chat tab
	</tab>
	<tab name="Activity" icon="icon-activity">
		this is the activity tab
	</tab>
	<tab name="Comments" icon="icon-comment">
		this is the comments tab
	</tab>
</app-sidebar>

@skjnldsv skjnldsv self-assigned this Apr 8, 2019
@skjnldsv skjnldsv added 3. to review Waiting for reviews enhancement New feature or request labels Apr 8, 2019
@skjnldsv skjnldsv added this to the next milestone Apr 8, 2019
@skjnldsv skjnldsv added the feature: app-sidebar Related to the app-sidebar component label Apr 8, 2019
@ChristophWurst

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@skjnldsv

This comment has been minimized.

src/components/AppSidebar/AppSidebar.vue Outdated Show resolved Hide resolved
src/components/AppSidebar/AppSidebar.vue Outdated Show resolved Hide resolved
@skjnldsv skjnldsv requested a review from korelstar April 9, 2019 06:39
@jancborchardt

This comment has been minimized.

@jancborchardt

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@korelstar

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@jancborchardt

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Apr 11, 2019

Okay, we're all good here!
here is the vuexample I'm using vueexample.tar.gz

I added some accessibility stuff according to the aria guidelines.
@nextcloud/accessibility I need some opinions on the tab/tab-content focus vidual feedback (blue border and blue shadow on tab content). When you're focusing the tabs, you can navigate with arrows and pageUp/Down. Pressing tab focus the tab content (aria guidelines) but we need a feedback for the user to know where the focus is. Same goes for the tabs navigation, we need to differentiate the focus and the selected tab.

Please test! 🙏 🙇‍♀️

To ignore:

@skjnldsv skjnldsv added the high High priority label Apr 11, 2019
@ChristophWurst

This comment has been minimized.

Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Tested and works nicely 👍

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Copy link
Contributor

@korelstar korelstar left a comment

Choose a reason for hiding this comment

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

Tabs should be optional. Currently, I'm forced to use them (otherwise I will get JavaScript errors). However, using tabs if there is only few content to display doesn't make any sense.

Alternatively, I could provide a single tab and you could hide app-sidebar-tabs__nav if there is only one tab. But then I have to use a fake name and icon for the tab.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Apr 11, 2019

@korelstar why would tabs be optional?
The idea is also to try to improve the sidebar content. Currently we have a lot of apps that push a lot of thing at the same location and looks messy.
Ideally, we should regroup and tabify the data displayed in the sidebar! :)

Though I agree, you could also have too little informations to need 2 tabs or more.
Though a clear separation is needed, those are not the best looking visual :(

Any ideas?

1 2
Capture d’écran_2019-04-11_16-11-21 Capture d’écran_2019-04-11_16-12-02

@korelstar
Copy link
Contributor

In general, I agree totally, that tabs are useful. It just looks weird if I need one tab only. But you're right, the second variant isn't the best. Maybe you could just add more space between button 1 and the (tab) content?

@jancborchardt
Copy link
Contributor

Yes, @korelstar has a good point – if there is only 1 section of content, the tabs are not needed.

The tab content fields should also be self-explanatory (chat will have chat history and an input box, and detail inputs will have labels etc.) so we can leave it if there is only one tab.

@skjnldsv
Copy link
Contributor Author

@jancborchardt so, we don't change anything and we left it like the second screenshot?
I also agree that no tabs are needed in that case, but I also simply sees that the ui is not really great as is, without tabs. 😉

@jancborchardt
Copy link
Contributor

@skjnldsv yup, I'd say 2nd screenshot is good!

(Of course with only 1 line of text it looks off, but that's also the case with the tab header. ;)

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Contributor Author

All done!
@korelstar if no tabs used, you can just put raw content without the tab component. :)

@skjnldsv skjnldsv merged commit a996cc8 into master Apr 15, 2019
@skjnldsv skjnldsv deleted the enhancement/sidebar branch April 15, 2019 07:11
@juliushaertl juliushaertl mentioned this pull request Apr 15, 2019
11 tasks
@korelstar
Copy link
Contributor

@skjnldsv

if no tabs used, you can just put raw content without the tab component. :)

No, this doesn't work, because you get an error "Error in mounted hook: "TypeError: this.tabs[0] is undefined"

But using a single tab is fine for me. This works as expected, now.

@skjnldsv
Copy link
Contributor Author

@korelstar ah, I might have forgot a check somewhere :)

@skjnldsv
Copy link
Contributor Author

@korelstar could you open a new issue with the full log stack? :)

@korelstar
Copy link
Contributor

Even better: I've provided a fix for that problem in #359. :-)

skjnldsv pushed a commit that referenced this pull request Apr 20, 2019
There was an error when a `AppSidebar` doesn't contain any `AppSidebarTab`s and uses just the content directly.

See #340 (comment)
@skjnldsv skjnldsv modified the milestones: next, 0.10.0 May 7, 2019
@skjnldsv skjnldsv mentioned this pull request Jul 5, 2019
6 tasks
@skjnldsv skjnldsv mentioned this pull request Oct 23, 2019
3 tasks
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-sidebar Related to the app-sidebar component high High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sidebar standardization
5 participants