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(deps): Readd fixed style import from nextcloud/dialogs #10855

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

nickvergessen
Copy link
Member

☑️ Resolves

🖌️ UI Checklist

src/App.vue Outdated Show resolved Hide resolved
@ShGKme
Copy link
Contributor

ShGKme commented Nov 15, 2023

PR updated.

@nextcloud/dialogs requires importing styles and supposed to be used as

import { showError } from '@nextcloud/dialogs'
import '@nextcloud/dialogs/style.css'

But we use dialogs in 60+ modules. It is easy to forget import styles each time.

Importing styles once per entry point is enough to use dialogs on the entry point.

In this commit, styles are imported only in entry points were they are needed by manual check.

  • Imported via common init.js
    • main: main.js
    • files-sidebar: mainFilesSidebar.js + mainFilesSidebarLoader.js
    • public-share-auth-sidebar: mainPublicShareAuthSidebar.js
    • public-share-sidebar: mainPublicShareSidebar.js
  • Imported manually
    • maps: maps.js
    • deck: deck.js
    • admin-settings: mainAdminSettings.js
    • recording: mainRecording.js
  • Not imported, don't use @nextcloud/dialogs:
    • dashboard: dashboard.js
    • flow: flow.js
    • collections: collections.js

This is not very reliable, because we may add a new entry point, or start to use dialogs in some existing entry point without dialogs, or stop using dialogs in some entry point.

But it's simple and clear.

Alternative solutions:

  • Import everywhere with webpack config no matter if @nextcloud/dialogs are used
  • Use @nextcloud/dialogs via wrapper module and forbid direct import by ESLint rule:
    // utils/nextcloudDialogs.js
    import '@nextcloud/dialogs/style.css'
    export * from '@nextcloud/dialogs'

@ShGKme ShGKme requested review from Antreesy and removed request for Antreesy November 15, 2023 12:09
`@nextcloud/dialogs` requires importing styles.
But we use nextcloud dialogs in 60+ modules, it is easy to forgot import styles each time.
Importing styles once per entry point is enough to use dialogs on the entry point.

In this commit, styles are imported only in entry points were they are needed by a manual check.

- Imported via common `init.js`
  - main: `main.js`
  - files-sidebar: `mainFilesSidebar.js + mainFilesSidebarLoader.js`
  - public-share-auth-sidebar: `mainPublicShareAuthSidebar.js`
  - public-share-sidebar: `mainPublicShareSidebar.js`
- Imported manually
  - maps: `maps.js`
  - deck: `deck.js`
  - admin-settings: `mainAdminSettings.js`
  - recording: `mainRecording.js`
- Don't use `@nextcloud/dialogs`:
  - dashboard: `dashboard.js`
  - flow: `flow.js`
  - collections: `collections.js`

Signed-off-by: Joas Schilling <coding@schilljs.com>
Co-authored-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the bugfix/10854/readd-style-import branch from 4608440 to 0c9599e Compare November 15, 2023 12:11
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Styles for toast messages and file pickers are loaded and referred to spreed repo.
There is another style source from server with 404, but it seems to not have an effect on Talk app

Tested for:

  • main.js
  • mainAdminSettings.js
  • init.js (file sidebar)

@nickvergessen nickvergessen merged commit 79fe3b4 into main Nov 16, 2023
@nickvergessen nickvergessen deleted the bugfix/10854/readd-style-import branch November 16, 2023 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix nextcloud/dialogs usage
3 participants