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(UF) : Add frontend for unified form #850

Merged
merged 116 commits into from
Aug 18, 2022

Conversation

tr1ten
Copy link
Collaborator

@tr1ten tr1ten commented Jun 4, 2022

front-end part of unified creation form

@tr1ten tr1ten changed the base branch from master to new-creation-form June 4, 2022 12:22
@tr1ten tr1ten marked this pull request as ready for review August 8, 2022 13:40
Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

It's so awesome to see the feature getting so well set up !
You're doing a great job !

I commented on lots of details, which we would expect for such a big PR 😄
For now I didn't review the tests (by the way, also awesome to see the long overdue test harness for the front-end!), I figured we should start with that for now.

On my side i feel like I'm failing you a bit by not having thought through an important aspect.
I know it is very late in the game for this comment, but here it is: In view of some of the added complications of having these entity stubs (new entites created with modals) to deal with, we could reconsider that when an entity is added through the create modal we actually submit the entity for creation at that point, like the MB editor does.
If we get a BBID back from that request we can use the BBID directly. That will avoid some state complications, removing the custom deduplication code, etc.

We would still need the save-multiple-entities route because we will be creating links between all these entities.
Please let me know how you think we could approach that.

src/client/components/author-credit-display.js Outdated Show resolved Hide resolved
src/client/containers/layout.js Outdated Show resolved Hide resolved
src/client/entity-editor/alias-editor/alias-modal-body.tsx Outdated Show resolved Hide resolved
import {omit} from 'lodash';
import {removeEmptyAliases} from '../../entity-editor/alias-editor/actions';
import {removeEmptyIdentifiers} from '../../entity-editor/identifier-editor/actions';
// import {validateAuthorSection} from '../../entity-editor/validators/author';
Copy link
Member

Choose a reason for hiding this comment

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

What was your final outcome with the validation state? Was it still slowing down the app too much? If so we'll unfortunately have to get rid fo the commented out code (and any other code that was just for this validation feature) [a moment of silence for the fallen bits].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Though after optimization this doesn't seem to cause any issue. but now i am not sure if we need validations on modal, let me know if you think we should keep the validations or not.

src/client/helpers/entity.tsx Outdated Show resolved Hide resolved
src/client/stylesheets/style.scss Outdated Show resolved Hide resolved
src/client/unified-form/validators/detail-tab.ts Outdated Show resolved Hide resolved
@tr1ten
Copy link
Collaborator Author

tr1ten commented Aug 10, 2022

Hey, thanks for such a detailed review.

we could reconsider that when an entity is added through the create modal we actually submit the entity for creation at that point, like the MB editor does.

If i understand it correctly you meant submitting the modal directly to server and not storing entity state for later submission?
I did think about doing the same later realize it might not be a good idea for the following reasons -

  1. It would be cumbersome to use the new entity for purposes like duplication where we need whole entity state.

  2. Since this editor is mainly targeted (but not limited) to new user, they might make mistake during edit which can easily be fixed from current workflow.

  3. Adding and removing attributes on entity is much easier now else we will need to create unnecessary revisions.

  4. In current workflow, Summary tab may help user to review their entities and possibly fix them if needed giving more quality submissions.

  5. lastly i believed that strategy might fall short in the long run when we start using UF editor for more complex workflow and may result in even more complication than we have now.

I apologize for not communicating my reasons well for current workflow properly during the proposal :.
If you still think these reasons don't worth the extra complication, then after some discussion on implementation details we can go ahead that strategy.

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

OK, after another review I'm happy to get this PR merged before it grows any larger :)

We still have some details to sort out which I will write out as separate feedback, but it can all be done in separate PRs now that we have this good functioning base.

Nice work ! :)
P.S: I've also deployed the current state of the PR to test.bookbrainz.org/create as requested

@MonkeyDo MonkeyDo merged commit 3b3b7e2 into metabrainz:new-creation-form Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants