-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add apps and infra details to technical-details page #490
Conversation
✔️ Deploy Preview for keen-clarke-470db9 ready! 🔨 Explore the source changes: d01e687 🔍 Inspect the deploy log: https://app.netlify.com/sites/keen-clarke-470db9/deploys/61046d3d3082ed0008b324d5 😎 Browse the preview: https://deploy-preview-490--keen-clarke-470db9.netlify.app |
c781838
to
e747422
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together Zack. However, a reminder to not copy text from GitHub/Slack comments verbatim, but to use interpretation, judgment, etc to turn those into a form a customer walking into this with 0 knowledge of Gruntwork could understand.
body: | | ||
Instead of spending months (or years) to re-invent the wheel, let us handle the Gruntwork for you, so you can focus on your core deliverables | ||
|
||
categories: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lite/pro/enterprise thing was for Heroku. HIPAA requires a different table with different features. From my original comment on this:
Perhaps we have a similar table for HIPAA, except the columns are “Gruntwork Standard,” “Gruntwork CIS,” and “Gruntwork HIPAA” and we highlight a bunch of the HIPAA requirements as rows you only get with the HIPAA subscription: e.g., CIS hardened base images, anti-virus, file integrity monitoring, etc. We would also X out any services not eligible for HIPAA (if any).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on the quoted suggestion. I believe we actually had a table very much along those lines at one point, so we can probably pull it back in and add some additional rows to flesh out the distinctions more clearly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Table implemented in 3afc7ed. Took a first pass at the copy, but this still needs some additional HIPAA infra requirements - and some of the items that originated from the index page's table should be dropped - but the styling and structure is in place now.
Looking back at my PR comments, I came across as a bit of an asshole here. I think I had the wrong tone here and didn't capture a lot of important context. Sorry about that! I'm going to try to record a video of doing a PR review in the future and see if that provides a more effective code review, capturing my tone, adding important context, and also showing what I look for and how I think about things. |
This page is now indeed quite long. I'd suggest one of two solutions. I like the first better, but it's more work (how much more?)
|
I think both approaches would work here. I can start on the tabbed navigation and report back. |
Got the basics working in 004b188. I think it works, but we probably want to consider some styling to make the two content tabs more obvious: i.e., I don't want anyone missing the two navigation pills below the hero - and therefore not understanding there are two content tabs they should tab between. @ebeneliason thoughts? |
@bwhaley We'd like your input on what belongs in the table describing specific HIPAA features of the infrastructure. Here's what it looks like on the page now. The first few rows are bespoke to this table, the remainder is just copied from the home page, and might not be appropriate. Here's a relevant quote from Jim:
Can you suggest anything else specific to highlight in this table? |
3afc7ed
to
09207dc
Compare
|
Thanks, Ben. I've implemented your suggestions. Is Anti-virus something we could theoretically provide for added value? Perhaps we could find a provider to partner with here? |
16dde81
to
3e41011
Compare
IMO the details on how to meet this won't be fully understood until our solution is reviewed by a qualified HITRUST CSF assessor. The requirement itself comes from _§164.308(a)(5)(ii)(B) - Protection from malicious software _ which could absolutely be interpreted as A/V software. However, HIPAA interpretations are subjective, and it's often more about meeting the spirit of the law than the letter of the law. In this case, practically speaking, does it make any sense at all to run A/V in an AWS environment? E.g. imagine that you're running an EKS cluster with tens of worker nodes, and with each node running some set of containers. Where does the A/V go? On the worker nodes? In the containers? And why? Is there a common, widespread risk of virus infection in that environment? IMHO, the answer is no, there's practically no risk of a virus. However, there are other malicious software risks. For example, we might want a data exfiltration solution to prevent malicious software from siphoning PHI from the environment. We should also checksum and scan container images, and keep worker nodes patched. All of these could be considered "protection from malicious software" and would provide more practical value than A/V. WDYT? |
- Fix issue causing tab selection to bleed across viewers - Move common CSS and JS into a single partial included once
Jim requested this - I do personally think it works given that we link alternately to the "apps" and the "infra" tab on the
Should be removed now!
Agreed - working on it
Yep - fixed!
Roger - working on it |
Okay, in that case I think we can make it work, but we shouldn't call it "products" — I think it should still be "Technical Details" or similar.
Sorry, I missed this before. And I know you're working on the styling here already. Question: how useful is the hero in this context? Should we consider omitting it and just putting a relevant blurb under the header to add context for each page/section? I'm not sure if the tabs are more or less visible with/without it… |
They are more visible without the hero there, and a lot of what's in the hero is repeated immediately below it - so I'll try dropping it out for now. |
Okay, one more nit, and a related question… Nit: Perhaps the Why Gruntwork? page should come next to last, before pricing, so the core details regarding what the offering is come first. Question: I know we're just using the template provided, but should we consider refactoring the way the nav works so that we don't have to modify every subpage just to change a link or swap their order? It should really be a single consistent thing across all pages, which an include is perfect for. |
I lied, more nits. :) Won't promise it's the last this time… I'd propose dropping the "HIPAA-compliant" prefix on those nav links. I think that detail is both somewhat implied by context, and better to reiterate in the page. It makes them long and harder to distinguish between quickly. Will "architecture" make sense as a thing for the viewer coming to this site without any knowledge of our product? Or would it be worth calling these e.g. "Infrastructure" and "Applications" (in the nav)? We could still keep the headers as they are, to introduce our spin on the solution to those two areas of interest. |
l
Alright - I changed my mind about the products dropdown after implementing this. Now all nav is defined in the |
Haha, I'm probably arguing against myself to some degree now, but as long as we're going to include both links, I actually liked them combined under one item so that we could have the more succinct, thin nav. I was accustomed to the tab appearance before, but I think this works. The selection style is definitely more clear! I wouldn't dim the text for the unselected tab on hover — I think the background change is enough affordance. My preference would also be to left align the tabs with the page content, but maybe you tried that and it didn't work well? Also, I'm still seeing inconsistent padding on those page headers. I'd add padding above the applications header to match architecture, and add a little more below both before the content. |
Roger!
|
Hopefully I'm looking at the latest. I still see a few things to tweak.
Looks great to me apart from those things. I you don't have time to address them all, I could potentially add a PR. Let's make sure we work to get what you have merged before EOD. I haven't done a full code review, but then again — do we need that level of scrutiny for this quick mockup? Probably not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked closely at the code, but the preview looks good and remaining nits can be fixed in a follow up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Thanks for the reviews! |
Closes #468
Closes #474
These changes add the Ref Arch details to the Technical Details page:
TODO
Copy and markup
Images
Create and add new images for ref-arch detail page - using this comment as a guide:
Bug fixes
/technical-details
page/technical-details
page using default nav - should be using hipaa nav