Header Detailpage #64 #11
Conversation
| export let opacity = '' | ||
|
|
||
| // Define different image sources for different resolutions or viewport widths | ||
| export let Small = 'header-image-small.webp' |
There was a problem hiding this comment.
Some developers in some languages use capitals for objects. Is there purpose behind the capitals on Small, Medium and Large? If so.. is this set as a rule in the code-standards you use?
|
|
||
| <picture> | ||
| <!-- Source for small screens --> | ||
| <source media="(max-width: 375px)" srcset={Small} /> |
There was a problem hiding this comment.
This is pretty nice but you only serve webp.. what if a browser doesn't support it? A good practice is serving a jpg as fallback next to a couple of different sized webp images..
| viewBox="0 0 24 24" | ||
| > | ||
| <path | ||
| stroke="none" |
There was a problem hiding this comment.
I'm used to setting these in a style element or on the SVG main.
| stroke-linecap="round" | ||
| stroke-linejoin="round" | ||
| stroke-width="2" | ||
| d="M12 .042a9.992 9.992 0 0 0-9.981 9.98c0 2.57 1.99 6.592 5.915 11.954a5.034 5.034 0 0 0 8.132 0c3.925-5.362 5.915-9.384 5.915-11.954A9.992 9.992 0 0 0 12 .042ZM12 14a4 4 0 1 1 4-4 4 4 0 0 1-4 4Z" |
There was a problem hiding this comment.
Did you pull this SVG through SVGOMG?
| import IconLocation from '../atoms/IconLocation.svelte' | ||
|
|
||
| // Mockdata voor de header text | ||
| const mockData = [ |
|
|
||
| <section class="content-container"> | ||
| <!-- First content row --> | ||
| <div class="title-label"> |
There was a problem hiding this comment.
this <div> could be a <header> element.. See: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/header
| </div> | ||
|
|
||
| <!-- Second content row --> | ||
| <h1 class="subtitle">{mockData[0].subtitle}</h1> |
There was a problem hiding this comment.
Ony one <h1> element is allowed. I think you want a different heading level here..
| import HeaderImage from '../atoms/HeaderImage.svelte' | ||
| </script> | ||
|
|
||
| <div class="header-container"> |
There was a problem hiding this comment.
That's a lot of nested <div>'s, is this really necessary or can you use a more semantic option instead..
| @@ -9,12 +9,12 @@ | |||
| <kruimelpad parentSlug /> | |||
| } --> | |||
|
|
|||
| <div> | |||
| <!-- <div> | |||
There was a problem hiding this comment.
You're allowed to delete this ;)
There was a problem hiding this comment.
Did you pull this through SVGOMG?
|
Thanks for your feedback! Sorry, I don't understand what you mean by "afterwards you can merge this branch", because you already did or am I confused? And now, do I need to make a new branch to implement those remarks? Have a nice weekend! |
Header Detailpage #64
What does this change? 🚀
Start for the component 'HeaderDetailPage'. This component will eventually be placed at the top of the Detailpage for WOGO. The code currently contains MockData. The component is therefore not yet complete, but I would appreciate receiving feedback on the HTML and CSS.
How Has This Been Tested? ✅
The looks 👀
Small screens
Large screens
How to review 💡
There are a few questions I would appreciate that could help me during a review:
Thank you in advance!