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

Log reviewer response #350

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Log reviewer response #350

wants to merge 26 commits into from

Conversation

MrRob100
Copy link
Contributor

@MrRob100 MrRob100 commented May 20, 2024

# Conflicts:
#	src/components/Modal/AjaxModalWrapper.vue
#	src/components/Modal/ModalManager.vue
#	src/components/Modal/SideModalBodyLegacyAjax.vue
#	src/pages/workflow/WorkflowLogResponseForModal.vue
# Conflicts:
#	src/stores/modalStore.js
@jardakotesovec
Copy link
Collaborator

@MrRob100 After code improvements I will also ask you to add it to storybook, so we have that modal snapshoted, which helps with awarness that such modal do exist and it also helps with preventing some regressions over time as its screenshots are always compared to baseline.

It will require following

  • create stories file, which need to have button which opens this modal via play function. That what ensures that opened modal is screenshoted
  • create mdx file with simple description, so it does appear in storybook - and it automatically is able to pick up the props and show them there

One of the best examples is ReviewerSubmissionPage.stories, as that also have modals that are being opened via play function:

export const OpenModalAccepted = {
	play: async ({canvasElement}) => {
		// Assigns canvas to the component root element
		const canvas = within(canvasElement);
		const user = userEvent.setup();

		await user.click(canvas.getByText('Read Round 1 Review'));
	},
	decorators: [
		() => ({
                        // this needs to be adjusted based on your specific modal to ensure the whole modal gets screenshoted
			template: '<div style="height: 1600px"><story/></div>',
		}),
	],
};

Happy to give more guidance on mattermost for storybook if needed, there is just couple of things to watch out so the modal gets properly screenshoted.

@MrRob100
Copy link
Contributor Author

I have redone this using the pkp-form component. In order to make the form footer sit on the bottom of the modal, I have had to add some styling to the Form component. I have checked and it doesn’t seem to have knock-on effects on the other forms. Also, there doesn’t seem to be support for a cancel button in the pkp-form component which is why I have left that out. I can alter it to allow for one if we like. Thoughts @jardakotesovec @Devika008

@MrRob100 MrRob100 marked this pull request as ready for review May 30, 2024 02:16
Copy link
Collaborator

@jardakotesovec jardakotesovec left a comment

Choose a reason for hiding this comment

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

Here is example for storybook - 815ea2a

This ensures that if you open that story it opens the modal - and as result it gets automatically screenshotted in CI to ensure it looks good over time.

You can see it in storybook (cd lib/ui-library && npm run storybook).

Please just refine it a bit with some description. Otherwise I think one simple story suffice.

src/components/Form/Form.vue Outdated Show resolved Hide resolved
src/components/Form/Form.vue Outdated Show resolved Hide resolved
@jardakotesovec
Copy link
Collaborator

@MrRob100 Thanks! Looking good, just need to do some testing. Would you be able to rebase it, so its up to date with main branches? Having difficulties to run it. There has been lots of merged lots of changes recently.

# Conflicts:
#	src/components/ListPanel/doi/DoiItemVersionModal.vue
Copy link
Collaborator

@jardakotesovec jardakotesovec left a comment

Choose a reason for hiding this comment

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

Thanks for the storybook addition. Just couple small tweaks! And also make sure it does not include changes outside of this work. Thank you!

src/components/Form/Form.vue Outdated Show resolved Hide resolved
src/components/ListPanel/doi/DoiItemVersionModal.vue Outdated Show resolved Hide resolved
src/pages/workflow/WorkflowLogResponseForModal.vue Outdated Show resolved Hide resolved
src/pages/workflow/workflowLogResponseForModalStore.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@jardakotesovec jardakotesovec left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, looks great!

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.

3 participants