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.

Sign up for GitHub

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

Jump to bottom

fix: conflict picker #1123

Merged
merged 4 commits into from
Mar 22, 2024
Merged

fix: conflict picker #1123

merged 4 commits into from
Mar 22, 2024

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Mar 22, 2024 •
edited
Loading

Needed for nextcloud/server#44409

  • Fix a few design papercuts in ConflictPicker
  • Fix closing/cancel button in ConflictPicker
  • Fix renaming of File objects in ConflictPicker
  • Supports Folders in ConflictPicker
  • Fix asyncComponent logic and TS typings in vue components
  • Fix ConflictPicker mounted and throwing logic
  • Upgraded a few dependencies

@skjnldsv skjnldsv added bug Something isn't working regression Regression of a previous working feature dependencies Pull requests that update a dependency file 3. to review Waiting for reviews design Related to the design labels Mar 22, 2024
@skjnldsv skjnldsv self-assigned this Mar 22, 2024
This was referenced Mar 22, 2024
Merged
Merged
susnux
susnux reviewed Mar 22, 2024
View reviewed changes
lib/components/ConflictPicker.vue
@@ -425,6 +450,14 @@ export default Vue.extend({
opacity: .5;
filter: saturate(.7);
}

:deep(.dialog__actions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a fix for NcDialog needed, there should be no need to use deep. Probably need to make actions flex without margin.
Then you could use flex: 0 1 fit-content on the buttons here and flex: 1 1 on the separator and no deep styles needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I think the NcDialogs never had a left Button in mind when designed.
Not sure this is something we cant to push forward.

So yeah, until then, I'll keep it here

Copy link
Contributor

Choose a reason for hiding this comment

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

yes was more of "loud thinking" for future tasks of mine^^

susnux
susnux reviewed Mar 22, 2024
View reviewed changes
package.json Outdated
@@ -87,7 +88,7 @@
"simple-eta": "^3.0.2"
},
"peerDependencies": {
"@nextcloud/vue": "^8.0.0-beta || ^8.0.0",
"@nextcloud/vue": "^8.11.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed for the changes? Otherwise this might introduce duplicated dependencies for using apps.
Libraries should only pin the lowest supported version, the highest compatible version is picked by NPM on install and shared with the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed no.
We can keep ^8.0.0-beta || ^8.0.0, it doesn't matter, 8.11.0 is installed in the lockfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it did matter.
I had to do this to update the nc/vue and see the latest changes on the NcDialog. Otherwise it was weirdly not working 🤔

Now that the lockfile is updated, I can revert I guess.
But this makes updating the dep complicated (check the auto dependabot updates, it never offered @nextcloud/vue since we changed that line)

susnux
susnux approved these changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Nice work!

@skjnldsv
fix: conflict picker
365adde 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv

This comment was marked as resolved.

Sign in to view
@skjnldsv
fix: axios and OC.appConfig usage
0152903 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@susnux
Copy link
Contributor

susnux commented Mar 22, 2024

wtfff

EDIT: reverted to Vue.extend, this is very weird

This is correct behavior. Vue.extend does not add types from props / data / etc. defineComponent does.
Meaning if any of the published types is not included Typescript will show this error.

But yes had this too in some components

@skjnldsv
fix(cypress): conflict resolution checks and cypress state
d143e41 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv
chore(dev-deps): update
426015e 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv skjnldsv enabled auto-merge March 22, 2024 18:00
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 22, 2024
@skjnldsv skjnldsv merged commit 82bf9f9 into main Mar 22, 2024
16 checks passed
@skjnldsv skjnldsv deleted the fix/conflict-picker branch March 22, 2024 18:01
emoral435
emoral435 reviewed Mar 25, 2024
View reviewed changes
Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

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

Reviewing late, but this LGTM!

@skjnldsv skjnldsv mentioned this pull request Apr 2, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@susnux susnux susnux approved these changes

@emoral435 emoral435 emoral435 left review comments

Assignees

@skjnldsv skjnldsv

Labels
4. to release Ready to be released and/or waiting for tests to finish bug Something isn't working dependencies Pull requests that update a dependency file design Related to the design regression Regression of a previous working feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@skjnldsv @susnux @emoral435