-
Notifications
You must be signed in to change notification settings - Fork 47
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
pkp/pkp-lib#9771 Move ORCID functionality into core application #339
Conversation
419064a
to
74706c0
Compare
2aefe54
to
ea5388d
Compare
@jardakotesovec, would you be able to review this? This is part of the larger ORCID integration work in pkp/pkp-lib#9771. Thanks! |
@ewhanson Yes, started today.. and got stuck on first thing :-). While testing the implementation - I noticed that when I click on Enable ORCID functionality multiple times - to also disable it - it requires two clicks to disable it. So I wanted to understand where is that issue coming from. Btw can you reproduce that issue? And that took while to track down. I did identify that if I remove the other FieldOptions (orcidSendMailToAuthorsOnPublication) it started working. Seemed like multiple FieldOptions somehow interfere there, but how?!?! I tracked it down to Vue-draggable, which when properly disabled that issue disappeared. I will create PR to properly disable VueDraggable when ordering is not enabled. Will continue tomorrow. Any other surprise I should expect? :-) |
Hey @jardakotesovec, Thanks for having a look. And yes, I can reproduce the issue and it was on my list of things to ask you about once this work was done! (It was also affecting the DOI settings as well). In terms of surprises, maybe at least one more 😂 When accessing the contributor form with the ORCID functionality enabled, closing the modal for the Here's a short screencast of the modal issue happening: Screen.Recording.2024-05-28.at.11.02.35.AM.mov |
@ewhanson Yes, thats known problem, will tackle it in second half of June. Created issue you can track for progress - pkp/pkp-lib#9992 |
import FieldOrcidMock from '../mocks/field-orcid'; | ||
import {http, HttpResponse} from 'msw'; | ||
|
||
export default { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to add couple of more stories to cover multiple states as possible. Whats great on storybook that it makes screenshot per every story - therefore more stories you do - more you have captured component state in screenshots and it helps catch any regressions.
Also having difficulties to see localisations - so that might need to be added to global.js. Hopefully get soon enough to automate this process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's a good idea. I'll look into what states should be reflected for this. Are there any components you think would be a good example to look at that we already have?
And I thought I'd already added the locale strings to global.js
but I hadn't. I've done that now and updated the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ewhanson ReviewerSubmissionPage.stories.js is quite good example as it shows how you can add play
function to the story, which for example can click on some button there, which displays the modal and the end result will be snapshotted.
Only tricky thing with modals is that chromatic does not detect their size - so the height needs to be set explicitely. Good strategy is via decorator, also can be seen in that ReviewerSubmissionPage.stories.js
95fd85f
to
ae0bd14
Compare
9a56ec2
to
3ca7d9f
Compare
No description provided.