Hi
This is hitting all screen edges on my mobile in portrait and the image is cut off on mobile landscape
In the html, you must not use div/span alone for text. What are meaningful elements you could use in that price box?
Curious here - what do you think would happen if someone clicked “change” if this was a real project?
The BEM looks fine on this, but don’t be afraid to start/use a new block if you need to. Like that pricing grey box could easily be its own component used elsewhere on a site.
Similarly, on things like buttons/links you might find there are classes established for those styles that are reusable, and a BEM class would only be used if they needed changing/modifying. Eg a project might have a .btn class you’d use, then you’d also give it a bem class scoped to this component to control things like margin top/bottom on that button
Marked as helpful
Looking at css I can see the source of the mobile landscape issue straight away. Never use this
width: 100vw;
height: 100vh;
Height instead of min-height is stopping content from growing.
And 100vw is always a bad idea because it doesn’t account for scrollbars or device notches.
Other thing I noticed in the css if you’re changing more in the media query than expected. Eg why would hover style not be on links all the time? There’s no reason to limit it like that.
Make sure you always include obvious focus-visible styles on all interactive elements as well - very important for keyboard users
Good luck
Marked as helpful
@tarasis
Posted
@grace-snow first thank you for the incredibly helpful notes. I've made some changes based on them. (Not BEM yet)
- Media Query Good point on the hover/focus states. The reason I put them in the media query was that the design only showed the desktop version as having those states. I simply took it too literally without thinking about it.
I've tidied that up and actually swapped a couple of other bits in to variables which are just changed in the media query.
- the
100vw/vh
Interesting and thanks for the heads-up, I hadn't seen that effect in Safari (Desktop & iOS), Firefox or ResponsivelyApp. I started with using 100% for both but it broke vertical centering, so I swapped to vh and didn't think about it. I think the issue was something else but I didn't spend the time investigating then.
- Don't use
Span/Divfor text ...
I will get that drilled into me yet. I mentally balk at the idea of p because I think paragraph, and a few words or even a single word isn't a paragraph. Just need to repeat until I get to the point of not using div/span
- What would happen on clicking change?
I don't know and hadn't considered it. A dialog appearing, originating from the change button and using z-index, to allow changing between a few options. I don't know if it would be a local change, or a remote routed change.
- BEM
Thanks for the useful notes on BEM.
@tarasis great, that all makes sense.
So if you think change would open a modal (or toggle the price to monthly maybe), it would be a button, not anchor tag 😉
Marked as helpful
@tarasis
Posted
@grace-snow 🤦♂️ <<< me right now
