Conversation
|
My first visual review of the page can be accessed on the About Us page on Figma (to the right of the final mockups) |
|
I have addressed the issues in from Danielle's visual review. |
|
@daniellex0 - I did my best to make the changes from your visual review. When you have a chance, if you can have a look and see what I missed or didn't get quite right. I finally have the sticky navigation working, and the read-more/read-less on mobile, so you can see those. Thanks! |
Here is my visual review,I will do a code review tomorrow. tl;dr: the page you made is absolutely stunning, and there are quite a few bugs in the tablet view(which I do not believe was specified in the figma). First off, I would like to say that I am really impressed by what you were able to accomplish with this page. Nitty gritty visual review:Desktop view:
Tablet view:
MOBILE VIEW:
I believe that all of these details can be fixed in another pull request. For now I would like to merge the pull request so that the site is live and we can split this up into many issues. |
|
Just for future reference, this is where the animated gif for the donation section is: civictechindex/CTI-website-frontend#60 (comment) |
This file is not needed. It is there just for test
jbubar
left a comment
jbubar
left a comment
There was a problem hiding this comment.
Really great code. Thanks for giving me the opportunity to review it. I learned a lot
|
|
||
| .header-text--about-us { | ||
| font-size: 18px; | ||
| max-width: 500px; |
There was a problem hiding this comment.
Do you need this max-width property? It makes it pretty strange at screen sizes larger than 530px
|
|
||
| // CSS Grid | ||
| .page-content-container-grid { | ||
| display: grid; |
There was a problem hiding this comment.
Your use of css grid is mind-blowingly cool
| .is-active { | ||
| color: #333333; | ||
| font-weight: 700; | ||
| color: black !important; |
There was a problem hiding this comment.
I heard that using !important is not always good practice. I see why you used it here.
I think it is not turning black because a:link, a:visited has more priority than a single class selector .is-active
maybe in a future pull request you can figure that out.
checkout this cool resource
| margin-top: 25px; | ||
| } | ||
|
|
||
| .read-more { |
| } | ||
|
|
||
| .sticky-nav { | ||
| grid-area: nav; |
| text-align: center; | ||
| } | ||
|
|
||
| .metrics-image-size { |
There was a problem hiding this comment.
not sure why this is not working...
| /* Below are the breakpoints and adjustments for different size monitors */ | ||
|
|
||
| // Bigger than mobile - 480px | ||
| @media #{$bp-mobile-up} { |
| @media only screen and (max-width: 850px) { | ||
|
|
||
| .stick-it { | ||
| right: 0; |
There was a problem hiding this comment.
This is a good solution for the overlap problem.. maybe make the max-width a bit larger... like 1000px and add a few px to the right.... just a thought
| margin-bottom: 150px; | ||
| } | ||
|
|
||
| .about-us-section-header { |
There was a problem hiding this comment.
both padding and margin are computed on this one. not sure if that was on purpose
sticky-test.html
Outdated
|
|
||
| <div class="sticky-container"> | ||
| <div class="sticky-element"> | ||
| This bit should be sticky |
There was a problem hiding this comment.
Do we need this whole file?
Did you push this by accident?
4 participants
Fixes #614
This isn't actually quite ready for a PR, as even I don't think that it's ready to merge today. However, since @daniellex0 has already done a very comprehensive visual review, I feel like the process has begun, hence the pull request.
Before a code review I need to finish two bits of JavaScript:
Additionally, we need to integrate the actual letter from the actual Executive Director.
But it's a start...