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

Fix: peer review landing page #287

Merged
merged 23 commits into from
Dec 12, 2023
Merged

Fix: peer review landing page #287

merged 23 commits into from
Dec 12, 2023

Conversation

lwasser
Copy link
Member

@lwasser lwasser commented Nov 20, 2023

This is an attempt the make the peer review landing page on our website a bit more user focused with a narrative.
it breaks the page into sections.

Current status:

  • This PR is ready to merge text wise and structurally. the redesign i think looks nice. All it needs is graphics. it can remain open until those are done which will likely be before xmas. Or we can opt to add filler graphics and merge it as is for now. which could also be ok. i can just make some basic graphics.

Copy link
Collaborator

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Editing for content


### Who can submit a package?

<!-- ### Who can submit a package?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless you are going to use this in the next week or so, I would remove all comments as they clutter the content and can always be resurrected from the repo history.

Copy link
Member Author

Choose a reason for hiding this comment

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

great point - i'll do that now!!

margin-bottom: 2em;
border-bottom: 1px solid $border-color;
margin-bottom: .02em; //used to be 2em
// border-bottom: 1px solid $border-color;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

@@ -85,8 +86,8 @@
max-width: 362px;
background-color: #f1f0f1;;
border-radius: 4px;
padding: 32px 24px;
margin: 12px;
// padding: 32px 24px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// padding: 32px 24px;

transition: transform 0.15s ease-out;
}

// &:before {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the block 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

@willingc 😆 you have caught me in my usual pattern of being nervous to sometimes delete things so i comment it out until i'm sure i want to really delete it! i'll go through and cleanup comments. this was me removing the circles on our people cards but i think i want to keep them for other content cards . so i'll make that fix locally!!

Copy link
Collaborator

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Looking good Leah. Most are small content tweaks.

@lwasser lwasser changed the title Fix: review landing page Fix: peer review landing page Nov 21, 2023
@lwasser
Copy link
Member Author

lwasser commented Nov 21, 2023

to view the rendered page you can go here

in the artifacts section of our circleci build!! i will swap out images with more fitting graphics once we have stable content . right now i just used the same image over and over as a placeholder.

@willingc
Copy link
Collaborator

It looks great @lwasser. ☀️

Copy link
Collaborator

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Looking good.

@lwasser
Copy link
Member Author

lwasser commented Nov 29, 2023

thank you so much @willingc will be making and integrating all of these changes in the next day.

@lwasser
Copy link
Member Author

lwasser commented Nov 29, 2023

the related fix to the author page in our peer review guide here

the idea being that an author hits the peer review page and then can be directed to the author guide in the "Ready to submit" section. That guide page then has relevant sections of the guide for them to review creating an author content pathway.

Screenshot 2023-11-29 at 3 51 07 PM

willingc
willingc previously approved these changes Nov 29, 2023
Copy link
Collaborator

@willingc willingc left a comment

Choose a reason for hiding this comment

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

CI is passing. I'm going to approve. @lwasser 🚢 it.

Copy link
Contributor

@kierisi kierisi left a comment

Choose a reason for hiding this comment

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

added some copy suggestions!

JOSS where you can submit your package to us for review. If your package is accepted
and if it is in scope for JOSS, it will then be fast tracked through JOSS'
review process; they will only review your paper.md file. Consider this a
win-win!
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
win-win!
excerpt: "Our collaboration with JOSS means that you don't have to chose between pyOpenSci and JOSS. Submit your package to pyOS for review, and if your package is accepted and in scope for JOSS, it will be fast-tracked through JOSS' review process; they will only review your `paper.md` file. It's a win-win!"

Copy link
Member Author

Choose a reason for hiding this comment

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

i want to commit this one but i suspect this edit needs to be applied to lines 38-42 and here it's replacing just line 42 with that entire blog of text. i'll come back to this edit !! the text is excellent!

@willingc
Copy link
Collaborator

willingc commented Dec 1, 2023

Awesome @kierisi. Love the alt text additions. @lwasser I will let you rereview content and it looks good to me.

@lwasser
Copy link
Member Author

lwasser commented Dec 4, 2023

ok updates!
✅ i've merged all of your input here @kierisi thank you so much for all of the content edits. I welcome a second round if you wish to look at things again
✅ i've cleaned up the styles for the page - specifically the text and block at the bottom was too wide to be readable.

what's left

  • new images for the page that align with our colors and "playful" style
  • alt tags for those images

The current rendered page can be seen here

@kierisi please in working on this page i realized the JOSS section had been removed so i added it back.
I also think we could have images that are a bit less square to balance the page out some.

i'm going to open a separate PR to update our partners page. @kierisi i suspect you'll have really good feedback for that page as well. it's a similar style but also needs some cleanup of messaging and brand. i might be able to create better diagrams now that i'm using canva more but it still has some AI generated images of people (we do want representations of people all over!!)

@lwasser lwasser added the reviews-welcome Please review this item! label Dec 4, 2023
@willingc
Copy link
Collaborator

willingc commented Dec 5, 2023

@lwasser @kierisi May I make a small suggestion about PRs...

When making changes that are fluid (I.e. text, images, styling), it's often easier to create multiple PRs instead of combining into one PR.

I typically ask myself:

  • Is this one change better than what is already there?
  • Can this change stand alone without other changes?
  • Is there any significant downside to one PR landing a day or two before the next PR? (I use the Issues to keep track of to do items.)

For example, I would recommend landing all of the content changes in this PR, create another PR for the images, and another for the styling.

willingc
willingc previously approved these changes Dec 5, 2023
Copy link
Collaborator

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Small typo and clarifying text. 🚢

@lwasser
Copy link
Member Author

lwasser commented Dec 5, 2023

@lwasser @kierisi May I make a small suggestion about PRs...

When making changes that are fluid (I.e. text, images, styling), it's often easier to create multiple PRs instead of combining into one PR.

I typically ask myself:

  • Is this one change better than what is already there?
  • Can this change stand alone without other changes?
  • Is there any significant downside to one PR landing a day or two before the next PR? (I use the Issues to keep track of to do items.)

For example, I would recommend landing all of the content changes in this PR, create another PR for the images, and another for the styling.

Thank you @willingc this is helpful. i will definitely think about this more carefully for future pr's. In this case when i went to edit the page there was a lot of inline HTML with styles applied there that made the content more difficult to review.

so i pulled that html out and tried to move all of the text to the front matter for simplified editing and then templated content via includes and styles that could be resused. it was easier for me to do this all at once but i can see how as a reviewer it would make for a more complex review. i'll think about how to parse those elements out separately in the future. it's just a bit tricky when some of these pages were created a bit haphazardly (by me!)

we have asana tasks locally to track updates to images. But we can't edit alt tags on the page until images are fixed. i'm open to suggestions that you have about how to chunk the work buckets out a bit more cleanly as well.

Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
willingc
willingc previously approved these changes Dec 11, 2023
Copy link
Collaborator

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Let's land this PR 😄

@willingc
Copy link
Collaborator

@lwasser Looks like there is a merge conflict here.

@lwasser
Copy link
Member Author

lwasser commented Dec 12, 2023

ok! the only downside of merging it now is there is one image i used 3-4 times on the page.

but i suppose we could just embrace a second pr that updates those images knowing not that many people will be on the page nor will they judge us for using an image several times? but i'll go ahead and merge and might just put other placeholder images in via a later pr (from the partners page) .

@lwasser lwasser merged commit d946a3f into pyOpenSci:main Dec 12, 2023
@lwasser lwasser deleted the fix-review branch December 12, 2023 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviews-welcome Please review this item!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants