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

Fix missing Talk sidebar trigger in public share pages #7593

Merged

Conversation

danxuliu
Copy link
Member

Follow up to #7576
Fixes #7578

Since the removal of SCSS files the menu-people icon can no longer be used directly with a CSS class. However, it is available as a Vue component; although it would be possible to directly render the Vue component for the icon* inside the button element the whole button is moved to Vue instead, as the other approach requires more fighting with the styles.

*For reference in case it is ever needed:

const talkSidebarTriggerIconVm = new Vue({
	// MenuPeople is a functional component, so it needs to be explicitly
	// rendered rather than just spread (...MenuPeople) in the constructor
	// of the Vue instance.
	render: h => h(MenuPeople, {
		props: {
			fillColor: 'var(--color-primary-text)',
			size: 20,
			decorative: true,
		},
	}),
})
talkSidebarTriggerIconVm.$mount('#talk-sidebar-trigger-icon')

@danxuliu danxuliu added 3. to review bug feature: frontend 🖌️ "Web UI" client feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents labels Jul 13, 2022
@danxuliu danxuliu added this to the 💚 Next Major (25) milestone Jul 13, 2022
@nickvergessen
Copy link
Member

It's not a functional component anymore with #7592 🤕😅

src/PublicShareSidebarTrigger.vue Outdated Show resolved Hide resolved
src/PublicShareSidebarTrigger.vue Outdated Show resolved Hide resolved
src/PublicShareSidebarTrigger.vue Outdated Show resolved Hide resolved
src/PublicShareSidebarTrigger.vue Outdated Show resolved Hide resolved
@nickvergessen
Copy link
Member

Keyboard tabbing before nextcloud-libraries/nextcloud-vue#2869

Bildschirmfoto vom 2022-07-21 16-19-00

Keyboard tabbing with nextcloud-libraries/nextcloud-vue#2869

Bildschirmfoto vom 2022-07-21 16-14-07

So let's get this in nextcloud-libraries/nextcloud-vue#2871

@nickvergessen
Copy link
Member

Vue lib is in

Copy link
Member Author

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Header menu toggles and app buttons have an opacity of 0.85, but the Vue button uses 0.7. Should the opacity be increased (in PublicShareSidebarTrigger)?

Other than the comments, tested and works 👍

Thanks for adding the style for the Vue button!


<style scoped>
.button-holder {
margin: 2px 5px 2px 2px;
Copy link
Member Author

Choose a reason for hiding this comment

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

Header contents are vertically centered. Are the vertical margins needed?

Out of curiosity, why 5px instead of 2px for margin-right?

Copy link
Member

Choose a reason for hiding this comment

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

The margins where needed otherwise the box shadow was cut off.

For the right sidebar I added 3px on top so the white border of keyboard active doesn't touch the browser as it looked too weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

The margins where needed otherwise the box shadow was cut off.

I tried removing the margins (or, rather, replacing with margin-right: 3px) and as far as I can tell it was working fine 🤷 That is why I was asking. But maybe it depends on the browser, I do not know.

For the right sidebar I added 3px on top so the white border of keyboard active doesn't touch the browser as it looked too weird.

👍

Comment on lines +69 to +70
display: flex;
justify-content: center;
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it needed to horizontally center the button? As the div has no explicit width it should have the width of its child elements, so the button should be already centerd in the div. Or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

Copied this from the app icons and only then the centering worked as expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above, I tried removing the rules and as far as I can tell it was working fine. But same as above, maybe it is related to the browser, no idea 🤷

@marcoambrosini
Copy link
Member

We should probably not rely on the opacity for the hover effect in the button in the first place @danxuliu

@danxuliu
Copy link
Member Author

We should probably not rely on the opacity for the hover effect in the button in the first place @danxuliu

And how it should be done? Using a background? Something else? In any case it should be changed in nextcloud-vue as well as in the header stylesheet.

@marcoambrosini
Copy link
Member

Yes I mean changing it in the vue library. The hover feedback can be just the change in cursor (we have a tertiary style that has a background feedback), this one should be used only for special cases. The keyboard navigation feedback is a big box shadow.

@nickvergessen
Copy link
Member

Since vue 5.4 is merged, this can be rebased and then merged.

@nickvergessen
Copy link
Member

Nevermind, one of the things that was merged after 5.4.0 nextcloud-libraries/nextcloud-vue#2940

Since the removal of SCSS files the "menu-people" icon can no longer be
used directly with a CSS class. However, it is available as a Vue
component; although it would be possible to directly render the Vue
component for the icon inside the button element the whole button is
moved to Vue instead, as the other approach requires more fighting with
the styles.

As the icon will be shown with a transparent background on the header,
which uses the primary color, the fill color of the icon needs to be
explicitly set to the primary text color.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@nickvergessen nickvergessen force-pushed the fix-missing-talk-sidebar-trigger-in-public-share-page branch from 8a1f953 to 089defd Compare August 11, 2022 07:05
@nickvergessen
Copy link
Member

Rebased

@nickvergessen
Copy link
Member

As per chat:

Also I'd vote to merge the sidebar trigger even though it currently shows wrong colors, but at least it can be used again

Updating the vue lib to 6 with #7711 will fix it.

@nickvergessen nickvergessen merged commit 4df033e into master Aug 11, 2022
@nickvergessen nickvergessen deleted the fix-missing-talk-sidebar-trigger-in-public-share-page branch August 11, 2022 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug feature: frontend 🖌️ "Web UI" client feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trigger on public share page is not visible
3 participants