-
Notifications
You must be signed in to change notification settings - Fork 48
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
I9992 #366
I9992 #366
Conversation
…ure that the form errors are propagated to the modal for other ListPanels
…some other modals for more consistent naming
…tor Dialog to expose the props for docs
v-if="currentAttacher" | ||
v-bind="currentAttacher" | ||
@selected:files="(...args) => emit('attachFiles', ...args)" | ||
@cancel="(...args) => emit('cancel', ...args)" |
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.
Just FYI: my IDE was labeling the @cancel
event as deprecated, but I couldn't find anything more about it.
<td>{{ dateRangeDescription }}</td> | ||
</tr> | ||
</table> | ||
<action-panel class="pkpStats__reportAction"> |
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.
I noticed here <action-panel>
and <pkp-button>
are used whereas other places (I noticed in PublicationsDowloadReportModal
but wasn't comprehensive in checking everywhere) <PkpButton>
and <ActionPanel>
are used. I know both will work, but wondering if we've standardized on a "house style" for this and if so, if it could be encapsulated in an eslint rule or something?
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 I am almost always aiming for vue defaults. Which in case is to use ActionPanel in vue SFC files and only to use action-panel when its defined in 'html', which in our cases would be in smarty templates that we are moving away from. Having eslint rule for this is good idea, will check that out.
<template> | ||
<p>{{ t('manager.dois.update.partialFailure') }}</p> | ||
<ul> | ||
<li v-for="errorMessage in failedDoiActions" :key="errorMessage.index"> |
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.
</div> | ||
|
||
<div class="doiListItem__versionContainer--actionsBar"> | ||
<spinner v-if="isSaving" /> |
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.
It seems to be working as intended, but neither <spinner>
nor <pkp-button>
on the next line have been imported. Not sure if it's an issue, but wanted to flag it for you.
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 Thanks - its because most component are registered globally. But I think still there is value to import dependencies always so its clear on which components it depends.. for example in storybook its important.
I wish this would be caught by tooling like eslint as well.. maybe its possible, will check.
@@ -421,3 +423,4 @@ export default { | |||
} | |||
} | |||
</style> | |||
./HighlightsEditModal.vue |
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.
I'm guessing this probably isn't supposed to be here.
<template> | ||
<SideModalBody> | ||
<template #title> | ||
{{ t('submission.wizard.changeSubmission') }} |
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.
Makes use of t
but doesn't have the corresponding const {t} = useLocalize();
in the script below.
src/components/Modal/SideModal.mdx
Outdated
|
||
## Usage | ||
|
||
We recommend to define modals as individual component files, rather than inline them, that ensures that the `setup` function and any other component life cycle event are triggered as modal is opened/closed. Therefore its easier to control when for example to fetch data from API. We might introduce option to define inline modals for basic use cases in future | ||
Its important to create modals as separate files, which works best with our [useModal](../?path=/docs/composables-usemodal--docs#opensidemodal) composable. Also having them defined as individual components ensures that the `setup` function and any other component life cycle event are triggered as modal is opened/closed. Therefore its easier to control when for example to fetch data from API. |
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.
Both its should be It's
. Therefore it's easier
could also be Therefore, it's easier
, but that's more of a style preference.
src/composables/useModal.mdx
Outdated
# useModal | ||
|
||
## openDialog | ||
|
||
Dialogs provide a quick way to show a simple confirmation prompt. Import `useModal` composable and use the openDialog() method to create a dialog. | ||
Dialog purpose is to display simple feedback like success and error messages. Or request confirmation for example before deleting some content. Import `useModal` composable and use the openDialog() method to create a dialog. Check [Dialog](../?path=/docs/components-dialog--docs) component for more details on available props. |
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.
Another minor wording suggestion: instead of Dialog purpose
, maybe The Dialog component's purpose
.
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.
Thank you, wording suggestions are much appreciated, not my strong suite :-)
No description provided.