Conversation
ItsLaszlo
requested review from
daniellen00,
daniellex0 and
jbubar
and removed request for
jbubar
_sass/components/_sitemap.scss
Outdated
| //¿¿Is there a class for page body specifiying color, height, flex-direction? | ||
| .sitemap-page { | ||
| background: #E5E5E5; | ||
| //following figma color but this is different from the rest of the site(#f7f5f5) |
There was a problem hiding this comment.
This should indeed be the standard color (#f7f5f5), I'm not sure why or where that other color is in Figma!
_sass/components/_sitemap.scss
Outdated
| } | ||
|
|
||
|
|
||
| //¿¿Do I need to create my own page specific class for the page cards or can I copy & paste the following notes left by Danielle? |
There was a problem hiding this comment.
Yes please use the page card classes that I included in the notes (if they work for you)- they will eventually be in a common shared scss file, but in the meantime these can live in your page's scss file
|
|
||
| /*--------------------------- | ||
| | Base Page Card | ||
| ---------------------------*/ |
There was a problem hiding this comment.
I'm concerned that these base classes are not nested. My understanding is this could cause unpredictable effects on other pages.
There was a problem hiding this comment.
Those classes are only temporarily there until @daniellex0 standardizes the page card classes. Do you believe it will still be an issue if I leave it as is until the classes get standardized?
|
@martham0 the page is looking fantastic, great job- it's so close! I have some CSS edits and it's easiest for me to communicate them via Figma if that's ok, so I wrote them down with screenshots to the the right of the my previous notes on the sitemap page Figma file. Yes even more of my crazy notes, sorry! Let me know if you have any questions or if any of them don't make sense.. |
|
The page has been updated with the edits mentioned above. Please look it over once more and let me know if you have any comments on the page @daniellex0 @trtincher. Thanks, Danielle for the extra notes/pictures on Figma. They were really helpful. |
There was a problem hiding this comment.
Wow this looks perfect, good job!! 🙌
My only nit-picky edit (I'm so sorry to do this) is that in mobile the paragraph text should be max-width 300px (it's currently 311px - this is the case on some other pages devs are working on right now so this might be some kind of bigger issue..). Is there any way to make the paragraph text max 300px width in mobile? I'm really sorry, I know this seems very minor but it affects legibility in mobile when the text is too close to the margins, and a few pixels make the difference.. but this is literally the ONLY edit I see, otherwise it's all fantastic.
|
Updates have been made @daniellex0 . Thanks for the notes!
|
drubgrubby
left a comment
drubgrubby
left a comment
There was a problem hiding this comment.
@martham0 - Really nice work! This page looks great. I just have a couple of small comments and then we're ready to merge.
| @@ -1,6 +1,6 @@ | |||
| .header-container { | |||
| background: #fff; | |||
| padding: 40px; | |||
There was a problem hiding this comment.
The class ".header-container" is used on other pages (about-us, events, getting started). If you make a change here it will impact all of those pages. I'm sure at some point Danielle will re-vamp all of the container classes, but in the meantime it seems better not to change the padding here, but to put a second class in "_sitemap.scss" that modifies this.
.header-container--sitemap {
padding: 64px;
}
Or you could move the class to sitemap.scss and nest it to limit it's scope.
There was a problem hiding this comment.
As I get further in I see that you already have that class with the changed padding, so you can change this class back to what it was originally. (If you use it as .header-container somewhere else that I haven't gotten to yet, then use the above suggestion to make a secondary class for that.
sitemap.html
Outdated
| --- | ||
| <!-- Header banner --> | ||
|
|
||
| <div class=" header-container--sitemap"> |
There was a problem hiding this comment.
There's an extra space after the quote, before the class name. (Nitpicky, yes, but while you're in there you might as well clean that up.)
sitemap.html
Outdated
| our team. | ||
| </p> | ||
|
|
||
| <a href="https://github.com/hackforla/website/wiki"><button class="btn btn-primary btn-md-long-2">Check out |
There was a problem hiding this comment.
I'm assuming that we want these buttons to open in a new tab. (@daniellex0)
sitemap.html
Outdated
| a GitHub account to open an issue. If you don’t have one, | ||
| you can always use the buttons on our Join page. | ||
| </p> | ||
| <a |
There was a problem hiding this comment.
I'm assuming that we want these buttons to open in a new tab. (@daniellex0)
|
@martham0 Visual review done, looks fantastic! 👍 |
|
Changes made and ready to deploy. |
4 participants
fixes #595
Created the sitemap page.
Web
Mobile:
This line of code needs to be changed before merged:
