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

Sidebar for New Display Package Page #8650

Merged
merged 9 commits into from
Jun 30, 2021
Merged

Conversation

sophiamfavro
Copy link
Contributor

@sophiamfavro sophiamfavro commented Jun 22, 2021

Solution

I rearranged the information in the sidebar and got rid of Authors to match the redesign mockups. This included moving the download information to the top of the sidebar, moving report to above the tags, and adding the full stats and contact owners link to be across from their respective header. I then went into the .less files and added the necessary styling changes to match the mockups. This included rounding the owner icons, changing the color and spacing of the section headers, and changing the font size of all the text.
Here is what it looks like now:
image

Here is what the whole page looks like:
image

Testing

In order to test my changes I checked the console to make sure nothing happened to the .js file. Inspected the elements to make sure that the corrected styling was being applied to them, and made sure all of the features worked with the feature flag on and off.

@sophiamfavro sophiamfavro requested a review from a team as a code owner June 22, 2021 00:27
@sophiamfavro sophiamfavro changed the title finished styling sidebar Sidebar for New Display Package Page Jun 22, 2021
@sophiamfavro
Copy link
Contributor Author

IN PROGRESS PHASE RIGHT NOW

@agr
Copy link
Contributor

agr commented Jun 22, 2021

Download item spacing looks uneven:
image
How does screen reader handle reading this information now that it is in table?

@dannyjdev dannyjdev self-requested a review June 23, 2021 17:55
@loic-sharma loic-sharma marked this pull request as draft June 23, 2021 18:20
@loic-sharma
Copy link
Contributor

loic-sharma commented Jun 23, 2021

@agr @dannyjdev Thanks for taking a look! I marked this pull request as a draft as we're still working on this. We'll let you know when this is ready for review! :)

@sophiamfavro sophiamfavro marked this pull request as ready for review June 25, 2021 17:09
@agr
Copy link
Contributor

agr commented Jun 25, 2021

If the screenshot on top is still actual, we need to be a bit more consistent with spacing:
image
(Before I started measuring, I only noticed the ones marked in blue, so it might be enough to just fix those. Is it because the font is smaller there?)

Also, the download lines look like being pushed to the left: those are the only lines that don't have icons.

@sophiamfavro
Copy link
Contributor Author

@agr I saw your comment so I went and double checked the spacing using edge inspect tooling. The spacing is consistent according to the developer tools, but it looks inconsistent due to the different font sizes. I will reach out to Jiachen and Maryanne to see if we can make the spacing in the downloads section smaller to make everything look more consistent.

@sophiamfavro
Copy link
Contributor Author

@agr I saw your comment so I went and double checked the spacing using edge inspect tooling. The spacing is consistent according to the developer tools, but it looks inconsistent due to the different font sizes. I will reach out to Jiachen and Maryanne to see if we can make the spacing in the downloads section smaller to make everything look more consistent.

I checked with Jiachen and Maryanne and we decided to make the download rows margins smaller to give it a more consistent look. Everything else matches the mockups and we will update them with this change accordingly.

.page-package-details-v2 .owner-list li {
margin-bottom: 8px;
margin-bottom: 38px;
Copy link
Contributor

@zhhyu zhhyu Jun 30, 2021

Choose a reason for hiding this comment

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

I am not very sure, but will it affect the package with multiple owners, and introduce more space between two owners?

Copy link
Contributor

@loic-sharma loic-sharma Jun 30, 2021

Choose a reason for hiding this comment

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

Great catch! This does add a little too much spacing between owners:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for catching this! I fixed the spacing so it should match the mockup and not be that far apart

@zhhyu
Copy link
Contributor

zhhyu commented Jun 30, 2021

I suggest to get other approvals before checking in.

@sophiamfavro sophiamfavro merged commit b0701d0 into dev Jun 30, 2021
@joelverhagen joelverhagen deleted the sofavroV2SidebarBackup branch August 22, 2024 16:28
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.

4 participants