Skip to content

fix: conflict picker#1123

Merged
skjnldsv merged 4 commits intomainfrom
fix/conflict-picker
Mar 22, 2024
Merged

fix: conflict picker#1123
skjnldsv merged 4 commits intomainfrom
fix/conflict-picker

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 requested review from emoral435 and susnux March 22, 2024 14:44
@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
susnux
susnux reviewed Mar 22, 2024
View reviewed changes
susnux
susnux reviewed Mar 22, 2024
View reviewed changes
lib/components/ConflictPicker.vue
filter: saturate(.7);
}

:deep(.dialog__actions) {
Copy link
Contributor

@susnux susnux Mar 22, 2024

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

@skjnldsv skjnldsv Mar 22, 2024

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

@susnux susnux Mar 22, 2024

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
},
"peerDependencies": {
"@nextcloud/vue": "^8.0.0-beta || ^8.0.0",
"@nextcloud/vue": "^8.11.0",
Copy link
Contributor

@susnux susnux Mar 22, 2024

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

@skjnldsv skjnldsv Mar 22, 2024

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

@skjnldsv skjnldsv Mar 22, 2024

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 skjnldsv force-pushed the fix/conflict-picker branch from 0b8d38e to 365adde Compare March 22, 2024 16:58
@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>
@skjnldsv skjnldsv force-pushed the fix/conflict-picker branch from 85f0506 to 0152903 Compare March 22, 2024 17:23
@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 skjnldsv force-pushed the fix/conflict-picker branch from 07d85a9 to 4a4c3f2 Compare March 22, 2024 17:53
@skjnldsv
chore(dev-deps): update
426015e 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the fix/conflict-picker branch from 4a4c3f2 to 426015e Compare March 22, 2024 17:59
@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
@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

+1 more reviewer

@emoral435 emoral435 emoral435 left review comments

Reviewers whose approvals may not affect merge requirements

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.

3 participants

@skjnldsv @susnux @emoral435