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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 11 commits into from
Jan 6, 2020
Merged

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

merged 11 commits into from
Jan 6, 2020

Conversation

mmmavis
Copy link
Collaborator

@mmmavis mmmavis commented Dec 19, 2019 •
edited
Loading

@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-4088 December 19, 2019 00:44 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 December 19, 2019 17:05 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 December 19, 2019 19:45 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 December 19, 2019 21:01 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 December 19, 2019 21:12 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 December 19, 2019 21:12 Inactive
@mmmavis mmmavis marked this pull request as ready for review December 19, 2019 21:26
@mmmavis mmmavis requested a review from beccaklam December 19, 2019 21:27
beccaklam
beccaklam previously requested changes Dec 19, 2019
View reviewed changes
Copy link

@beccaklam beccaklam left a comment •
edited by mmmavis
Loading

Choose a reason for hiding this comment

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

@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 December 19, 2019 22:53 Inactive
@mmmavis
Copy link
Collaborator 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 requested review from beccaklam and Pomax December 19, 2019 22:58
@beccaklam
Copy link

@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 December 20, 2019 00:27 Inactive
@mmmavis mmmavis requested a review from Pomax December 20, 2019 00:28
@mmmavis
Copy link
Collaborator Author

mmmavis commented Dec 20, 2019 •
edited
Loading

@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
Copy link
Contributor

Pomax commented Dec 20, 2019 •
edited
Loading

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

Pomax
Pomax reviewed Dec 20, 2019
View reviewed changes
network-api/networkapi/buyersguide/views.py
@@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Pomax
Pomax approved these changes Dec 20, 2019
View reviewed changes
Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

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

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-4088 December 20, 2019 00:47 Inactive
@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-4088 January 3, 2020 18:52 Inactive
beccaklam
beccaklam reviewed Jan 3, 2020
View reviewed changes
Copy link

@beccaklam beccaklam left a comment

Choose a reason for hiding this comment

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

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 January 3, 2020 19:18
beccaklam
beccaklam approved these changes Jan 3, 2020
View reviewed changes
Copy link

@beccaklam beccaklam left a comment

Choose a reason for hiding this comment

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

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

@mmmavis
Copy link
Collaborator 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
Copy link
Collaborator 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
@mmmavis mmmavis deleted the issue-4041-pni-mms branch January 6, 2020 17:24
@beccaklam
Copy link

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
Copy link
Collaborator 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
Reviewers

@Pomax Pomax Pomax approved these changes

@beccaklam beccaklam beccaklam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PNI product page: updates to MMS section
4 participants
@mmmavis @beccaklam @Pomax @patjouk