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: migrate conflictpicker to NcDialog and remove incorrect semantic closing icon #1113

Merged
merged 3 commits into from
Mar 15, 2024

Conversation

emoral435
Copy link
Contributor

Resolves #1095

Summary

  • Before, the ConflictPicker was built using NcModal. This is good, but migrating to NcDialog will allow for more features if needed in the future, and we also can remove the "x" button
  • The reason this PR was opened was because we could not click it, but after looking more into it, I do not believe that the conflict picker should have it. Previously, it did not even close the modal anyways (it would open it again), and semantically, because a conflict is happening, I believe that the user should choose between a file, or ignore importing the file.

Video

firefox_9NKju1QCHo.mp4

@emoral435 emoral435 added bug Something isn't working enhancement New feature or request 3. to review Waiting for reviews labels Mar 13, 2024
@emoral435 emoral435 self-assigned this Mar 13, 2024
@@ -372,7 +373,7 @@ export default Vue.extend({
position: sticky;
z-index: 10;
top: 0;
padding: var(--margin);
padding: 0 var(--margin);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lessen the padding for the header, as without this, the header has more padding than previously and looks really bloated

Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Contributor

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Thanks!

lib/components/ConflictPicker.vue Outdated Show resolved Hide resolved
@emoral435 emoral435 force-pushed the fix/migrate-conflict-picker-to-NcDialog branch from d1107ff to 54fe138 Compare March 14, 2024 13:11
@emoral435 emoral435 requested a review from susnux March 14, 2024 13:12
@emoral435
Copy link
Contributor Author

emoral435 commented Mar 14, 2024

Cypress seems related - will fix 👍

@emoral435 emoral435 force-pushed the fix/migrate-conflict-picker-to-NcDialog branch from a3a66eb to 87da200 Compare March 15, 2024 13:35
@emoral435
Copy link
Contributor Author

Fixed Cypress tests - also updated one section to reflect that we no longer have the default modal close button, and instead should always use the skip button 👍

@emoral435 emoral435 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 15, 2024
@emoral435 emoral435 merged commit 5d3ceac into main Mar 15, 2024
15 checks passed
@emoral435 emoral435 deleted the fix/migrate-conflict-picker-to-NcDialog branch March 15, 2024 13:36
@skjnldsv
Copy link
Contributor

skjnldsv commented Mar 21, 2024

@emoral435 this broke a feature.
Now we cannot cancel an upload.

The close icon was used to know if the user wanted to stop the process entirely.
There are two buttons:

  1. One to skip the conflicts, but it will still upload the files that you selected and are NOT conflicting
  2. The continue button, to continue after resolving the conflicts (and proceed to the initial upload)

I will fix it in a followup and add a third dedicated cancel button

EDIT: #1123
After discussion with design team on public design Talk channel

@emoral435
Copy link
Contributor Author

@emoral435 this broke a feature.
Now we cannot cancel an upload.

Ahh, I see. I will check out the PR you mention in the comment, but thank you for the catch! :)

@skjnldsv skjnldsv mentioned this pull request Apr 2, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[a11y] Close button is not clickable when ConflictPicker is opened
4 participants