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

Port dav calendar settings page to Vue.js #27008

Merged
merged 1 commit into from
Jun 5, 2021

Conversation

francoisfreitag
Copy link
Contributor

@francoisfreitag francoisfreitag commented May 18, 2021

  • Drop reliance on deprecated global jQuery object.
  • Allow testing user interactions.
  • Use newer technology stack.

Add infrastructure to test Vue components:

@francoisfreitag francoisfreitag marked this pull request as draft May 18, 2021 07:27
@francoisfreitag francoisfreitag force-pushed the no-jq-app-calendar branch 4 times, most recently from bfbd5f3 to 1fc7aa1 Compare May 18, 2021 08:17
@francoisfreitag

This comment has been minimized.

@artonge
Copy link
Contributor

artonge commented May 18, 2021

Nice @francoisfreitag,

Could you add some before/after screenshots ?

| Before | After |
|:---------:|:------:|
|xxxxxxx|xxxxx|

How do translations get collected? IIUC, this should be automatic?

This is done by the community, so you don't have to think about it.

apps/dav/src/views/CalDavSettings.vue Show resolved Hide resolved
apps/dav/webpack.js Show resolved Hide resolved
babel.config.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
apps/dav/src/views/CalDavSettings.vue Outdated Show resolved Hide resolved
@francoisfreitag
Copy link
Contributor Author

Thanks for the reviews!

Before After
Screenshot from 2021-05-30 11-13-56 Screenshot from 2021-05-30 11-18-20

Changes

Pulled relevant bits in separate commits, will eventually squash all commits.


I believe all feedback has been addressed and this PR is ready for another round of reviews.

@francoisfreitag francoisfreitag marked this pull request as ready for review May 30, 2021 20:39
@francoisfreitag francoisfreitag force-pushed the no-jq-app-calendar branch 2 times, most recently from e6561eb to 53f624e Compare May 31, 2021 08:53
apps/dav/lib/Settings/CalDAVSettings.php Outdated Show resolved Hide resolved
apps/dav/src/views/CalDavSettings.vue Show resolved Hide resolved
@francoisfreitag francoisfreitag force-pushed the no-jq-app-calendar branch 2 times, most recently from 827f684 to 5d186cc Compare May 31, 2021 09:24
@francoisfreitag
Copy link
Contributor Author

Changes

  • Squash commits
  • Use InitialState instead of deprecated OCP\IInitialStateService

Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

Apart from the js config global changes, LGTM

@francoisfreitag francoisfreitag force-pushed the no-jq-app-calendar branch 2 times, most recently from efc57e2 to 4cbd965 Compare May 31, 2021 20:04
@francoisfreitag
Copy link
Contributor Author

francoisfreitag commented May 31, 2021

Changes

  • Add missing enable of birthday calendar checkbox in tests.

@francoisfreitag
Copy link
Contributor Author

Apart from the js config global changes, LGTM

Can you precise your expectation here?

Are you asking to rewrite the existing JS so that it can be bundled as CommonJS modules? (#27022 and #23973)

- Drop reliance on deprecated global jQuery object.
- Allow testing user interactions.
- Use newer technology stack.

---

Test user interactions with the groupware dav settings

Add infrastructure to test Vue components:

- Use recommended libraries:

    - https://vuejs.org/v2/guide/testing.html#Recommendations
    - Use jest-dom for robust assertions on the DOM state
    - Use user-event to be more representative of user actions

- Code is transpiled by Jest, with the help of vue-jest.

Ignore test files for no-unpublished-import. Prevent ESLint from
flagging:

```
/home/runner/work/server/server/apps/dav/src/views/CalDavSettings.spec.js
Error:   1:24  error  "@testing-library/vue" is not published         node/no-unpublished-import
Error:   2:23  error  "@testing-library/user-event" is not published  node/no-unpublished-import
```

Signed-off-by: François Freitag <[email protected]>
@francoisfreitag
Copy link
Contributor Author

francoisfreitag commented Jun 5, 2021

Changes

diff --git a/apps/dav/src/views/CalDavSettings.spec.js b/apps/dav/src/views/CalDavSettings.spec.js
index 6f25106bd6..3f9254a301 100644
--- a/apps/dav/src/views/CalDavSettings.spec.js
+++ b/apps/dav/src/views/CalDavSettings.spec.js
@@ -92,6 +92,7 @@ describe('CalDavSettings', () => {
                        return Promise.resolve()
                })
                await userEvent.click(generateBirthdayCalendar)
+               expect(generateBirthdayCalendar).toBeEnabled()

                OCP.AppConfig.setValue.mockClear()
                await userEvent.click(sendEventReminders)

@skjnldsv
Copy link
Member

skjnldsv commented Jun 5, 2021

Can you precise your expectation here?

I think he meant the js files changed outside of /dav/. But it's because of the babel config update. All good

@skjnldsv skjnldsv merged commit b211d02 into nextcloud:master Jun 5, 2021
@skjnldsv skjnldsv added this to the Nextcloud 22 milestone Jun 5, 2021
@francoisfreitag
Copy link
Contributor Author

Awesome, thanks for the reviews and integrating!

@francoisfreitag francoisfreitag deleted the no-jq-app-calendar branch June 6, 2021 12:07
@MorrisJobke MorrisJobke mentioned this pull request Jun 10, 2021
59 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 feature: dav
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants