-
Notifications
You must be signed in to change notification settings - Fork 1
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
First parts of scoreset wizardification. #197
Conversation
…archPage Add a Clear All button in search page.
…d fix 3 resulting bugs: header buttons not working, badge numbers not centered, and search icon not displaying correctly.
Ultimately it will be good if we can again use a single screen for these two actions, but at least initially development will be easier if we leave the editor alone. Ideally all of the fields that can be edited after publication will be grouped on the later steps.
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.
This is much nicer than the previous experience, left a couple questions/comments I had while going through the code alongside creating a new score set with the wizard flow.
src/components/layout/Toolbar.vue
Outdated
@@ -18,6 +18,19 @@ | |||
</div> | |||
</div> | |||
</template> | |||
<template #item="{item, props, hasSubmenu}"> | |||
<router-link v-if="item.route" v-slot="{href, navigate}" :to="item.route" custom> | |||
<a v-ripple v-bind="props.action" class="p-menuitem-link" :href="href" @click="navigate"> |
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.
Not a huge deal, but it seems like these ripple animations need to be enabled globally to be active (https://primevue.org/ripple/).
When running this branch I see the below in the console
Toolbar.vue:31 [Vue warn]: Failed to resolve directive: ripple ...
We could also probably just remove the directive, not sure it is used anywhere else.
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.
removed
src/components/layout/Toolbar.vue
Outdated
<span class="p-menuitem-text">{{ item.label }}</span> | ||
</a> | ||
</router-link> | ||
<a v-else v-ripple class="p-menuitem-link" :href="item.url" :target="item.target" v-bind="props.action"> |
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.
v-ripple
directive, as above.
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.
removed
</span> | ||
<span v-if="validationErrors.primaryPublicationIdentifiers" class="mave-field-error">{{ | ||
validationErrors.primaryPublicationIdentifiers }}</span> | ||
<Message v-if="experiment" severity="info"> |
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.
Question on the location of this, which is currently right under the primary publication identifier dropdown. Would it make more sense at the top or bottom of the page? (Perhaps not, but thought it was worth mentioning).
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.
Moved to top.
<div class="mavedb-wizard-step-controls-row"> | ||
<div class="flex justify-content-between mavedb-wizard-step-controls pt-4"> | ||
<Button label="Back" severity="secondary" icon="pi pi-arrow-left" @click="showPreviousWizardStep" /> | ||
<Button label="Next" :disabled="this.maxWizardStepValidated < activeWizardStep" icon="pi pi-arrow-right" iconPos="right" @click="showNextWizardStepIfValid(showNextWizardStep)" /> |
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.
This step is automatically valid, but we should probably enforce that the user has added at least one target.
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.
done by your suggested change
this.targetGene = emptyTargetGene() | ||
}, | ||
|
||
addTarget: function () { |
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.
We'll need to integrate #194 here. I can handle that in the other PR once this one is merged in.
Co-authored-by: Benjamin Capodanno <[email protected]>
Populating target properties directly on autocomplete selections to avoid null reference errors when autofilling the target. Changes were incorporated from #194.
…name to data usage guidelines to match 'other' text.
…ry publication selector if there are not two or more publications. Fix bug where it the selector would show null if the last publication was deleted.
…e DataTable seems to fix (and hopefully has no undiscovered side effects).
By @jstone-uw and @ashsny.