I have finally gotten my first project in decent enough shape to submit for any possible feedback. I used flexbox to create the layout. The actual content of the page is a bit thin, but I will add some additional sections/verbiage as I become more comfortable with manipulating the layout.
Any suggestions, critiques, etc. would be greatly appreciated.
Hi Keith. Goo job on the styling and responsive design of this page. Good job providing the caption for the image as well. Here are some things that I noticed that could be further refined and improved.
- The text can get pretty small before it wraps onto another row. You could potentially solve this by either setting a minimum width on the content to prevent it from shrinking, using flex basis of the minimum width and allowing it to grow but not shrink, or adding a media query to switch the flex-direction to column.
- Since the header is spanning 100% of the width, another approach is to have the flex box only be the content rather than including the header as well.
- I noticed that you were using ids on your elements to target elements for your CSS styles. I would encourage you to use CSS classnames for targeting the styles. Ids add a higher level of CSS specificity that can be harder to override as your project grows. Also, using ids heavily increases the likelihood of having two elements with the same id which should be avoided.
- For the
PORTO H1, you can also add
text-transform: uppercase; as a CSS attribute to avoid having to type the header in uppercase. There is nothing wrong with your current approach, just another way to do things which comes in handy if say a content editor is entering the title text, but you always want it uppercased.
Again these are all fine tuning improvements. You are definitely on the right track. Keep up the good work.
Thank you for taking the time to look over my project and provide these pointers. I am working on making these revisions and have a question. I completely agree with you on your third bullet point, and this is how I would normally set up a project; however, I receive errors when running the test script on the page if ids are not used on a few of the elements as stated in the instructions for the challenge. Is there any way around this?
@keithstubbs Nice project! Mobile view is looking great.
Here are my suggestions on how to improve the page.
- I am wondering why the tribute-page div is not lined up with the image. It appears uneven to me. I have placed a thin red outline around the div to show you. Obviously the tribute div contains more content, but if elements are uneven it should be done with a purpose or make it look like it was done on purpose.
- For a travel destination, I think the picture should be more prominent and larger. If you get a picture with high resolution make sure the size of the file is not too big so your web page will load fast.
How much time have you worked upon this?
Brandon, thanks for the tips. I tried a few things to address your first item. I initially attempted to force the top of the heading text in the #tribute-info div to vertically align with the top of the image by setting a negative margin-top to the h2. This worked ok, but if I have to go back and change the font type or font size, I will have to adjust this margin. I added align-items: center to #main (flex container) to allow the #tribute-info and #img-div divs to align vertically along the cross axis, which I think looks a little better.
Am I on the right track on this or is there a better visual adjustment I need to make?
I added min-width: 300px; and flex: 2 300px; to the #tribute-info div to address the issue of your first bullet point.
Regarding your second bullet point, is excluding the header from the flex box a simpler option? I understand that keeping it in the container maybe redundant or unneccessary, but I like the idea of the entire site being contained within one container. That’s may not be a good reason to do it, though.
Thank you! It took me a couple of days to get the setup I wanted with flexbox, which I had not used before. I hope the next one I set up won’t take me as long.
@keithstubbs How about making the picture the full width of the page and drop the text below? Have the text “Porto second city of Portugal” be white text on top of the image. You can create rectangle of a transparent dark background behind the text to make it stand out more using hsla or rgba. This is just my opinion.
@brandon_wallace That is a great idea. I changed the image and it is now spanning the entire width of the page. I removed the header and placed the title text on top of the image. My intention was to have the title text positioned absolutely 20% from the top of the image, but I notice when viewing this on a mobile device, the title text moves down quite a bit. It seems that the text is being positioned 20% from the top of the document instead, and I cannot figure out how to change that. I added media queries to adjust the title position based on screen size, which isn’t as elegant a solution as I had hoped for.
Revised page: https://codepen.io/keithstubbs/pen/mZrEpL
Here are a few more tips.
- You should have one h1 font on your page for SEO.
- To stop the font from jumping at different widths you can use the calc() function. Like this
font-size: calc(4vw + 1rem);
What this does is it increases the font gradually as the width increases. The 1rem is there to set the default font size. It will not shrink less than 1rem even when the view width is zero. Adjust the numbers to suit your design.
@brandon_wallace I changed the h1 by using the calc function, and I like the effect. For your first statement, are you saying that I need to have a separate font for the h1 or that I need to remove the media queries in the CSS to resize the h1 (which I did when I added the calc function)?
@keithstubbs I was saying with number two that while I expanded the width of you page the h1 text would suddenly jump to a larger size because of the media queries and at times the text would wrap down to a second line. Using the calc() function you will have a very smooth increase of the h1 text. Yes, correct you would no longer have to apply a media query to the h1 element.
@brandon_wallace Got it. Thanks.
@keithstubbs Keep up the good work my brother.