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

Add ConflictModal and SyncConflictModal components #466

Merged
merged 19 commits into from
Nov 16, 2022
Merged

Add ConflictModal and SyncConflictModal components #466

merged 19 commits into from
Nov 16, 2022

Conversation

AbdulrhmnGhanem
Copy link
Member

@AbdulrhmnGhanem AbdulrhmnGhanem commented Oct 27, 2022 •
edited
Loading

Fixes #441

@AbdulrhmnGhanem AbdulrhmnGhanem mentioned this pull request Oct 27, 2022
Merged
@kasbah
Copy link
Member

kasbah commented Nov 3, 2022

Since react-testing-library seems to be letting us down I'm ok with not adding a test (if we are sure it's working) or adding a e2e test for this and being done with it for now.

@kasbah
Copy link
Member

kasbah commented Nov 3, 2022

One last thing maybe worth trying is to install @testing-library/jest-dom alongside @test-library/user-events and just get some of the examples from user-events to work without changing them. Then go from there...

Seems you can use jest-dom without jest: https://www.linkedin.com/pulse/snippet-vite-react-vitest-testing-library-lucian-crisan-

@AbdulrhmnGhanem AbdulrhmnGhanem marked this pull request as ready for review November 4, 2022 12:12
kasbah
kasbah requested changes Nov 4, 2022
View reviewed changes
Copy link
Member

@kasbah kasbah left a comment

Choose a reason for hiding this comment

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

  • Looks good overall except for some repeated logic
  • Nice job on the tests and getting them working
  • We shouldn't offer overwriting I think, but happy for us to open an issue for that and defer changing it to the next milestone.

.github/workflows/lint-frontend.yml Outdated Show resolved Hide resolved
e2e/cypress/integration/uploadProject.spec.js Show resolved Hide resolved
frontend/package.json Show resolved Hide resolved
frontend/src/components/NewProject/Upload/index.jsx Outdated Show resolved Hide resolved
frontend/src/components/NewProject/ConflictModals.tsx Outdated Show resolved Hide resolved
frontend/src/components/NewProject/Sync/index.jsx Outdated Show resolved Hide resolved
frontend/src/components/NewProject/Sync/index.jsx Outdated Show resolved Hide resolved
frontend/src/utils/index.js Outdated Show resolved Hide resolved
frontend/src/components/NewProject/Sync/index.jsx Outdated Show resolved Hide resolved
@kasbah
Copy link
Member

kasbah commented Nov 4, 2022

Oh and can you give this PR a better name please.

@AbdulrhmnGhanem AbdulrhmnGhanem marked this pull request as draft November 5, 2022 15:58
@AbdulrhmnGhanem AbdulrhmnGhanem force-pushed the conflict-modal branch 4 times, most recently from 0517f42 to a498137 Compare November 7, 2022 13:42
@AbdulrhmnGhanem AbdulrhmnGhanem changed the title conflict modal Add HOC ConflictModal and SyncConflictModal Nov 7, 2022
@AbdulrhmnGhanem
Allow choosing repo name when mirroring a repo in gitea
611d70b
@AbdulrhmnGhanem
feat: handle project name conflict when syncing projects
4a52127
@AbdulrhmnGhanem
eslint: use tyepscript parser
cbc22de
@AbdulrhmnGhanem
Refactor: extract the conflict modal into a seperate component
16f8ddc
@AbdulrhmnGhanem
Refactor: make upload conflict similar to sync conflict
863f714
@AbdulrhmnGhanem
Replace slugify with formatAsGiteaRepoName
ffe1952
@AbdulrhmnGhanem
Refactor: rename condlictModals.jsx -> conflictModals.jsx
f668c2e
@AbdulrhmnGhanem
test: add ConflictModals.text.tsx skeleton
dc07e01
@AbdulrhmnGhanem
eslint: @typescript-eslint/no-empty-function: off
a4e558a
@AbdulrhmnGhanem
Deps: add next-router-mock
53208ef
@AbdulrhmnGhanem
test: set up mocks for ConflictModals.test.tsx
5242774
@AbdulrhmnGhanem
test: add `opens conflict modal if the project name conflict with exi…
2620417 
…sting project`
@AbdulrhmnGhanem
test: changes the overwrite button to OK if the new project name does…
18529d8 
… not cause conflict
@AbdulrhmnGhanem
Fix: make event handling in useForm compatible with RTL fireEvent
51a9980
@AbdulrhmnGhanem AbdulrhmnGhanem marked this pull request as ready for review November 7, 2022 16:54
@kasbah
Copy link
Member

kasbah commented Nov 7, 2022 •
edited
Loading

Add HOC ConflictModal and SyncConflictModal

Does HOC stand for higher order component? So a component that takes in another component as an argument and transforms it? Do ConflictModal and SyncConflictModal do that?

@AbdulrhmnGhanem AbdulrhmnGhanem changed the title Add HOC ConflictModal and SyncConflictModal Add HOC ConflictModal and SyncConflictModal component Nov 7, 2022
@AbdulrhmnGhanem
Copy link
Member Author

We are using ConflictModal to create SyncConflictModal and UploadConflictModal.
I edited the name to clarify that SyncConflictModal is a normal component.

@AbdulrhmnGhanem AbdulrhmnGhanem changed the title Add HOC ConflictModal and SyncConflictModal component Add ConflictModal HOC and SyncConflictModal component Nov 7, 2022
@AbdulrhmnGhanem AbdulrhmnGhanem changed the title Add ConflictModal HOC and SyncConflictModal component Add ConflictModal and SyncConflictModal components Nov 7, 2022
@kasbah kasbah merged commit 79745aa into master Nov 16, 2022
@AbdulrhmnGhanem AbdulrhmnGhanem deleted the conflict-modal branch November 17, 2022 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kasbah kasbah kasbah requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Name conflict on import/sync repo has no resolution
2 participants
@AbdulrhmnGhanem @kasbah