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

PNI - update minimum security standards section on product page #4088

Merged
merged 11 commits into from Jan 6, 2020

Conversation

@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-4088 Dec 19, 2019 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 Dec 19, 2019 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 Dec 19, 2019 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 Dec 19, 2019 Inactive
mmmavis added 2 commits Dec 19, 2019
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 Dec 19, 2019 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 Dec 19, 2019 Inactive
@mmmavis mmmavis marked this pull request as ready for review Dec 19, 2019
@mmmavis mmmavis requested a review from beccaklam Dec 19, 2019
Copy link

beccaklam left a comment

@mmmavis This is looking great!

Just a few things:

  • The "Overall Security Rating" is currently inside a helptext div so it's font style is italic but it should be normal. I have it as Zilla Slab Light in the sketch file but if it's cleaner not to override I'm okay keeping it as whatever 'normal' is.

  • On mobile (not tablet) can we change the left/right padding for the section from 3rem to 1rem?

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 Dec 19, 2019 Inactive
@mmmavis

This comment has been minimized.

Copy link
Member Author

mmmavis commented Dec 19, 2019

@beccaklam

The "Overall Security Rating" is currently inside a helptext div so it's font style is italic but it should be normal. I have it as Zilla Slab Light in the sketch file but if it's cleaner not to override I'm okay keeping it as whatever 'normal' is.

The rating's denominator and numerator are both in custom types already so I've overridden/customized them further according to your feedback above (so they match what are in the Sketch file).

On mobile (not tablet) can we change the left/right padding for the section from 3rem to 1rem?

I guess you mean padding within the grey box? If so, I've made some adjustments to it.

@mmmavis mmmavis dismissed beccaklam’s stale review Dec 19, 2019

PR updated!

@mmmavis mmmavis requested review from beccaklam and Pomax Dec 19, 2019
@beccaklam

This comment has been minimized.

Copy link

beccaklam commented Dec 19, 2019

@mmmavis The changes are better than I expected! One last thing:

Can the grey box be full width on mobile, just like the image box? Right now there's like 1px of white space on the left and right sides:

Screen Shot 2019-12-19 at 6 11 43 PM

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 Dec 20, 2019 Inactive
@mmmavis mmmavis requested a review from Pomax Dec 20, 2019
@mmmavis mmmavis dismissed Pomax’s stale review Dec 20, 2019

PR updated!

@mmmavis

This comment has been minimized.

Copy link
Member Author

mmmavis commented Dec 20, 2019

@Pomax I've updated the PR based on your suggestions. Not sure if you are done for the year yet. If so, I can ask another dev to do the follow-up code review tomorrow instead.

Happy holidays! 🎉

(Note I still need to fix the styling issue Becca mentioned above but that can be reviewed by another dev.)

@Pomax

This comment has been minimized.

Copy link
Collaborator

Pomax commented Dec 20, 2019

Are these elements all looking the way they're supposed to? The question marks look serify, and the big number/small number is pretty sweet, but the star feels out of place?

image

@@ -117,12 +117,32 @@ def product_view(request, slug):
if product.draft and not request.user.is_authenticated:
raise Http404("Product does not exist")

product_dict = product.to_dict()

This comment has been minimized.

Copy link
@Pomax

Pomax Dec 20, 2019

Collaborator

this has tripped me up so much in the past =D

@Pomax
Pomax approved these changes Dec 20, 2019
Copy link
Collaborator

Pomax left a comment

python/template code wise this looks pretty good to me.

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 Dec 20, 2019 Inactive
@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-4088 Jan 3, 2020 Inactive
Copy link

beccaklam left a comment

The padding looks great @mmmavis !
Sorry one last question, would it be possible to make it 1/5 instead of 1.0/5? I think that would make it easier to scan. I can open a separate ticket if that's preferable.

@beccaklam beccaklam self-requested a review Jan 3, 2020
Copy link

beccaklam left a comment

Going to approve this for now though since the 1.0/5 is not a style change!

@mmmavis

This comment has been minimized.

Copy link
Member Author

mmmavis commented Jan 6, 2020

@beccaklam

Sorry one last question, would it be possible to make it 1/5 instead of 1.0/5? I think that would make it easier to scan. I can open a separate ticket if that's preferable.

Wow interesting find! Now I'm curious why 1.0 is the only case that's having this issue. I would say let's open a new ticket for that and assign it to me so we can get updates in this PR landed first.

@mmmavis

This comment has been minimized.

Copy link
Member Author

mmmavis commented Jan 6, 2020

Interesting. I just updated this PR with master and now I can't reproduce the 1.0/5 issue anymore.

image

@mmmavis mmmavis merged commit 740b461 into master Jan 6, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
percy/foundation.mozilla.org Visual review approved by Mavis Ou
Details
@mmmavis mmmavis deleted the issue-4041-pni-mms branch Jan 6, 2020
@beccaklam

This comment has been minimized.

Copy link

beccaklam commented Jan 6, 2020

Interesting. I just updated this PR with master and now I can't reproduce the 1.0/5 issue anymore.

image

Haha that's great?

@mmmavis

This comment has been minimized.

Copy link
Member Author

mmmavis commented Jan 6, 2020

I'll keep an eye on this in case the same issue appears again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.