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(workTable): Add work table on the edition page and edition table on the work page. #BB-330 #250

Merged
merged 7 commits into from Feb 13, 2019

Conversation

@akhilesh26
Copy link
Contributor

commented Feb 7, 2019

.

  1. Work table contains all the work contained by the edition.
  2. Add edition table in the work page, the table contains all the editions who containnig that work.
  3. Remove some unused lines of code of import section.

Problem

Solution

Areas of Impact

feat(workTable): Add work table on the edition page and edition table…
… on the work page.

1. Work table contains all the work contained by the edition.
2. Add edition table in the work page, the table contains all the editions who containnig that work.
3. Remove some unused lines of code of import section.
@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

Hi @MonkeyDo,
Please review this PR when you have time. If you will review this PR by today I will make the changes tomorrow morning. Otherwise I am on journey bw 8-11 Feb. But don't worrie, review this according to your time schedule.
Thanks :)

@coveralls

This comment has been minimized.

Copy link

commented Feb 7, 2019

Coverage Status

Coverage decreased (-0.2%) to 40.601% when pulling dd3e297 on akhilesh26:work_table into 881fcd6 on bookbrainz:master.

src/client/helpers/entity.js Outdated Show resolved Hide resolved
src/client/helpers/entity.js Outdated Show resolved Hide resolved
src/client/helpers/entity.js Outdated Show resolved Hide resolved
@MonkeyDo
Copy link
Contributor

left a comment

Thanks for this PR!
I think it's a great improvement and the entity pages feel more coherent, and it is clearer and more enticing for editors to contribute more data. Well done !

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

Thanks, @MonkeyDo! for review this PR and helping in some designing things for the work table and edition table. I created all of the changes. Any other changes also welcome!

@MonkeyDo
Copy link
Contributor

left a comment

That's working nicely, and I like the call to action.
I think it's enticing for users instead of having just a mostly empty page.

Good work !

src/server/helpers/utils.js Outdated Show resolved Hide resolved
src/client/helpers/entity.js Outdated Show resolved Hide resolved
src/client/helpers/entity.js Outdated Show resolved Hide resolved
@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

@MonkeyDo thanks for the detailed review. I created the changes that you mention above. Sorry for all little mistakes. More comments also welcome for any improvements.
Thanks a lot!

@MonkeyDo
Copy link
Contributor

left a comment

Sorry, I wasn't clear, my bad.
One last change :)

if (modelType === 'Work') {
return ['disambiguation', 'workType'];
return additionalRelations.concat(['disambiguation', 'workType']);

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Feb 13, 2019

Contributor

Sorry, I wasn't very clear with this.
I didn't mean to say to use concat here, but rather to always return an array, so that in another file I can call myArray.concat(getAdditionalRelations(...)).

So you can return array as you did before, but change the last line to be return [].

@MonkeyDo

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Thanks for taking all the changes on board so easily, and so quick too.

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

@MonkeyDo,
I created change as you commented. Thanks!

@MonkeyDo

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Great, thanks for this feature @akhilesh26, it's ready to merge and I'll deploy it to test.bookbrainz in the same breath. Eager to see it online !

@MonkeyDo MonkeyDo merged commit 8ea4454 into bookbrainz:master Feb 13, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 40.601%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.