Please Review - Tribute to Most Bad-ass Woman Ever

Hello there. I had a good deal of trouble with this – more so than I would have thought.

Please have a look. It isn’t what I intended, but it is what what I ended up with. So there’s that.

Please critique. I’ve reasonably thick skin. And thank you kindly.

6 Likes

Hi @BenjaminHilton I like your project…

1 Like

Thank you @sabir. Do you have any constructive criticism?

Honestly, it’s one of the best tribute pages I’ve seen. Lots of polish, very professional, and it’s responsive. Only thing I noticed was your title could probably use a min-width since it does get extra-small when the width goes very narrow, though I understand doing that could cause problems with the title overlapping her face. Perhaps just move the title above the image at very narrow widths?

1 Like

Seriously the best tribute project I’ve seen (including mine). Excellent styling.

Some issues with your choices re: responsivity and DRYing out your CSS

  • between 490 and 600 px, you have the 3 “panel” divs named “top” “middle” and “bottom” placed in a row beneath the <article>. They end up being clipped horizontally.
    • As a rule of thumb, if you take the right edge of a browser window and vary it from 4k to 325 px wide, there should never be a horizontal scrollbar. Vertical scrollbars are fine.
    • All 3 items have redundant styling. They should be a single class, like .panel so you “Don’t Repeat Yourself” and make it far easier to read your CSS.
  • Like @chuckadams said, your text can get tiny at certain widths (like ~494px, the “panels” text is small, and since “International Honors” already wraps and looks fine, don’t be shy about wrapping.

I know these things may seem nitpicky, but a big issue is that horizontal scrollbar (when over half of website views come on mobile), and a bigger issue (for the development of your “thinking like a programmer”, for pageload speed) is using classes. It makes your code more elegant, and makes it way more maintainable. Get into the habit now.

Beautiful and great page… I like everything about it…

1 Like

Thank you everyone for the kind words. And I absolutely welcome the nitpicky feedback. Truly.

@chuckadams I agree with your assessment of the text being too small. I just made the containing div a min-width and let the text overlap the hair and face; I think it’s still readable. If y’all disagree, I may just put a gradient textbox underlay with virtually no opacity, see if that does the trick.

@vipatron I realized the horizontal scroll bar was a result of me trying to get the header image to stretch across the page (right now it does not). I simply deleted the -10px margins and it seemed to do the trick. Further, I specify a panel class; I think this is a good deal more elegant. I kept the id’s as is, in the even that each one need be styled.

On thing I could not fathom, was the jQuery. I found a simple script that counts the numbers; what I couldn’t figure out was how to get it to trigger once the containing div appears on the screen (it doesn’t do much good if the numbers are already counted before the user scrolls there).

Thanks again everyone; I truly appreciate the feedback.

New changes in the link.

I think you’re looking for the onload event

It just may be, but I’m so criminally ignorant of javascript at this time. I was investigating waypoints, thinking I could trigger the event when the class enters the viewport. But so far I’ve been unable to make it work.

This is one of those problems that would require me to write a library that is a pared-down version of odometer.js. Load and use that if you want something that slick. If simpler does it for you, the top answer to this class of question on Stackoverflow has a link to the answerer’s fiddle using jQuery

Looks pretty good.

  1. I would stack the aside into a one-column layout sooner.

  2. You have a typo in the aside selector in your last media query (line: 310)

You have:
jusitfy-content: space-evenly;

Should be:
justify-content: space-evenly;

  1. You have two dublicate selectors #img-text, #bottom h3 is declared twice with different styles.

  2. This will kind of do what you want with the scroll, I didn’t use Jquery for my code but whatever.

Summary
(() => {
  const aside = document.querySelector("aside");
  window.addEventListener("scroll", animateNumbers);

  function animateNumbers() {
    if (aside.offsetTop < window.scrollY + aside.offsetHeight) {
      window.removeEventListener("scroll", animateNumbers);
	  // Your code starts here
      $(".count").each(function() {
        $(this)
          .prop("Counter", 0)
          .animate(
            {
              Counter: $(this).text()
            },
            {
              duration: 4000,
              easing: "swing",
              step: function(now, fx) {
                $(this).text(Math.ceil(now));
              }
            }
          );
      });
    }
  }
})();

Anyway, not bad I like it.

Nice project @BenjaminHilton,

Nice uses of images!

Thank you kindly for the critiques. I did stack the aside a little sooner, right around 600px width. I think it works better now.

I found the typo and remedied that as well.

I have #img-text and #bottom h3 declared more than once as they are individually styled for responsiveness. Is there a preferred method of accomplishing this?

And the JS code worked perfectly. Thank you for this as well. In time I will even understand how it works.

Thank you for the kind words, @gauravkukade!

That is really nice of you to say, @bmutebi!

@nataliecardot I’m left wondering what it is you could’ve said here.

The duplicate selectors I’m talking about is.

For the #img-text selector:

Line: 33 and Line: 54

For the #bottom h3 selector:

Line: 160 and Line: 171

They should be combined into just one selector each.

I saw your first comment, and let it go, figuring, “she’s smart enough to know what she said is wrong, since she deleted it so quickly.”

Definition of apologist from Collins dictionary: seriously: the very FIRST example
OED: First example of an apologist

You seem young and inexperienced, Ms. Cardot. We’ve been over this, as a society. Some light reading for you from a Symposium on Nazi Apologism

That’s war for you. It sucks. Anyway, let’s keep it focused on the technical merits of the page. Please?

Absolutely right, of course. Thank you kindly again!