Conversation
| :root { | ||
| --font-sans: | ||
| "Syne Variable", system-ui, -apple-system, "Segoe UI", "Open Sans", | ||
| "Helvetica Neue", Roboto, Arial, sans-serif; | ||
|
|
||
| --radius: 16px; | ||
| --bg: #f3e4c9; | ||
| --fg: #594439; | ||
| --alt: #bfa28c; | ||
| --accent: #babf94; | ||
| --shadow: 10px 10px rgba(125, 93, 77, 0.15); | ||
| --focus: 5px 5px rgba(125, 93, 77, 0.15); | ||
| } | ||
|
|
||
| /* Dark Mode Colors */ | ||
| :root.dark-mode { | ||
| --bg: #1a1a1a; | ||
| --fg: #e8d4c4; | ||
| --alt: #bfa28c; | ||
| --accent: #262726; | ||
| --shadow: 10px 10px rgba(232, 212, 196, 0.25); | ||
| --focus: 5px 5px rgba(232, 212, 196, 0.25); | ||
| } | ||
|
|
||
| body { |
There was a problem hiding this comment.
Very good use of the custom properties!
rafasilpereira
left a comment
rafasilpereira
left a comment
There was a problem hiding this comment.
Great cover of the accessibility with 0 issues! This is a differencial in market nowadays and a developer that thinks about it different is a differencial!
| transition-duration: 0.01ms !important; | ||
| animation-duration: 0.01ms !important; | ||
| animation-iteration-count: 1 !important; | ||
| scroll-behavior: auto !important; |
There was a problem hiding this comment.
Why did you used !important here? I recomend the read of this article about the use of this
There was a problem hiding this comment.
That is a great article!
So is !important here not correct? Because in the article they mention that it can be used for prefers-reduced-motion media query
There was a problem hiding this comment.
My question it was more to make you aware of this property. Many developers use it everywhere and do not consider when it's important to use like your case :D
| required | ||
| ></textarea> | ||
| </div> | ||
| <button type="submit" class="submit-button" id="submit-button"> |
There was a problem hiding this comment.
When I click submit, I'm not seeing the confirmation message. Do you know why?
There was a problem hiding this comment.
I realized that <div id="form-message"></div> was missing from index.html
Fixed in the new commit!
| } | ||
|
|
||
| /* Tablets/Medium Devices - 900px and below */ | ||
| @media (max-width: 900px) { |
There was a problem hiding this comment.
The assignment asks to be mobile-first, and you are covering first Desktop, then the others for mobile. How would you change this?
There was a problem hiding this comment.
That was my mistake, I only saw it after the assignment. The new styles.css file is now mobile-first. The difference is in the adjusted media query:
...
/* Desktop - 600px and above */
@media (min-width: 600px) {
...
|
|
||
| /* Contact Area */ | ||
| .contact { | ||
| grid-area: contact; |
There was a problem hiding this comment.
The assignment asks for this: "The contact form fields should stack vertically using Flexbox, with consistent spacing using gap". And you are using grid
There was a problem hiding this comment.
Honestly I preferred the look of the grid layout for it 😅 It is changed in the new commit, now it is vertical flexbox
| height: 6em; | ||
| } | ||
|
|
||
| textarea:focus { |
There was a problem hiding this comment.
You are not using focus-visible request from assignment
There was a problem hiding this comment.
I changed it now, but I don't see any noticeable difference between focus and focus-visible?
There was a problem hiding this comment.
Have you tried to navigate with TAB? The focus-visible is used when you navigate with TAB in the screen
| height: 48px; | ||
| width: 48px; | ||
| color: var(--accent); | ||
| background-color: #e8d4c4; |
There was a problem hiding this comment.
Why is this use a hard coded color?
There was a problem hiding this comment.
Initially I wanted to color the svg icon for the theme-toggle button but it always stayed at #000000 no matter what I do.
If I kept background-color: var(--bg) the icon will then not be visible in dark mode. That is why I hardcoded the color so it always stays the same and does not changed based on the theme
rafasilpereira
added
the
Reviewed
- refactored to mobile-first approach - made contact form vertical flexbox - added form submit message - changed focus to focus-visible - removed !important keyword
|
@rafasilpereira thank you for reviewing the assignment! I have made changes based on your feedback in a new commit. The fixes should be there now 👌 |
2 participants
The previous pull request contained duplicate files from cloning week 1 assignment repo.
I have also refactored the code to be cleaner and more readable, while implementing some finishing touches. Screenshots have also been included in this PR as per the assignment requirements.