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

feat: bring back cypress command createDescription through UI #4965

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

luka-nextcloud
Copy link
Contributor

📝 Summary

🖼️ Screenshots

🏚️ Before 🏡 After
B A

🚧 TODO

  • ...

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@@ -144,9 +143,10 @@ export default {
})
},
getFileInfo(autofocus) {
if (!this.hasRichWorkspace) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate a bit why this change is needed? as far as I see this would no longer check on each folder change on if the folder has a rich workspace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I checked,

  • when Readme.md file was created, it did not trigger updated function (at https://github.com/nextcloud/text/blob/main/src/helpers/files.js#L232) to update property hasRichWorkspace's value to true.
  • So, the RichWorkspace component was not be shown even though Text::showRichWorkspace was triggered since the property hasRichWorkspace was still false (until page refreshed)

Comment on lines 192 to 194
setTimeout(() => {
emit('Text::showRichWorkspace', { autofocus: true })
}, 200)
Copy link
Member

Choose a reason for hiding this comment

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

Can you tell why the previous one didn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When user directly accessed an empty folder, the render() (at https://github.com/nextcloud/text/blob/main/src/helpers/files.js#L207) did not run. So, RichWorkspace component was not created.
Then, triggering Text::showRichWorkspace right after creating Readme.md did not work.
The RichWorkspace component was created only after any file or folder was created.
So, we have to wait some time until RichWorkspace component is created.

Copy link
Member

Choose a reason for hiding this comment

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

Ah i see, that seems tricky. Let's get this in then for now. We might want to have the files app adjusted so that the header component itself is also rendered for empty file lists mid term. I only pushed a commit to use the file created event instead which seemed more clean.

@@ -12,5 +12,6 @@ registerDavProperty('nc:rich-workspace', { nc: 'http://nextcloud.org/ns' })
registerDavProperty('nc:rich-workspace-file', { nc: 'http://nextcloud.org/ns' })

if (workspaceAvailable) {
addMenuRichWorkspace()
Copy link
Member

Choose a reason for hiding this comment

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

Good idea to move that over here 👍

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Would be great to get a bit of further insights on the changes (see inline)

@juliusknorr juliusknorr force-pushed the feature/bring-back-rich-workspace-creation branch from 9f16fd3 to f73bd2f Compare November 13, 2023 09:25
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Code changes look good to me 👌

luka-nextcloud and others added 4 commits November 13, 2023 23:29
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
This will ensure that we can create the workspace in the new file menu
plugin and show it without requiring another propfind/reload (which is
not available through the new files app apis). Once the file list is
rendering the header we can use the flag to properly indicate the state
to the file header plugin

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the feature/bring-back-rich-workspace-creation branch from f73bd2f to 2cfa5e4 Compare November 13, 2023 22:29
@juliusknorr juliusknorr merged commit 76b6a4f into main Nov 14, 2023
35 checks passed
@juliusknorr juliusknorr deleted the feature/bring-back-rich-workspace-creation branch November 14, 2023 00:05
@juliusknorr
Copy link
Member

@luka-nextcloud just FYI I found a reasonable workaround for updating the view without the timestamp (see last commit)

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.

Cypress: Bring back create rich workspace through UI
3 participants