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

Use a11y activation util #38477

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use a11y activation util #38477

wants to merge 1 commit into from

Conversation

Pytal
Copy link
Member

@Pytal Pytal commented May 26, 2023

Summary

Use util from @nextcloud/vue and drop core util

Checklist

Signed-off-by: Christopher Ng <chrng8@gmail.com>
@Pytal Pytal added 3. to review Waiting for reviews accessibility labels May 26, 2023
@Pytal Pytal added this to the Nextcloud 28 milestone May 26, 2023
@Pytal Pytal self-assigned this May 26, 2023
@@ -49,7 +49,7 @@ import $ from 'jquery'

import OC from './index.js'
import OCA from '../OCA/index.js'
import { isA11yActivation } from '../Util/a11y.js'
import { isA11yActivation } from '@nextcloud/vue'
Copy link
Contributor

Choose a reason for hiding this comment

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

This will increase the dist bundle size by about 5MB.
Webpack is not good at tree shaking, so the best would be to use import ... from '@nextcloud/vue/dist/.../' but the a11y is currently not exported like this.

BTW: The other option to use the named import schema in every core file, is even worse...

Before this With this Everything as named import
57872356 62524760 73743763

(bytes)

Copy link
Contributor

Choose a reason for hiding this comment

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

5MB seems to be a lot

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this already known (one of the reasons I am pushing ESM + vite).

But for now we need to add an entry point for this to the @nextcloud/vue library and stick with the explicit default import.

JuliaKirschenheuter

This comment was marked as duplicate.

Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

I'm voting for usage of import ... from '@nextcloud/vue/dist/.../' too

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants