chec / commercejs-nextjs-demo-store

Commerce demo store built for the Jamstack. Built with Commerce.js, Next.js, and can be one-click deployed to Netlify. Includes product catalog, customer login, categories, variants, cart, checkout, payments (Stripe) order confirmation, and printable receipts.
https://commercejs-demo-store.netlify.app/
BSD 3-Clause "New" or "Revised" License
1.07k stars 206 forks source link

No option to NOT invert logo #108

Closed LB22 closed 4 years ago

LB22 commented 4 years ago

This project is really nice to get started with next.js and commerce.js. I would like to contribute all I can, but I'm a noob. Hopefully I can contribute as I learn, but seemingly not yet. I have a proposal, and it might be a small and easy fix, but I can't do it.

It is indeed stylish that the header is transparent and inverts color of the fonts/logo/shopping cart when you're all the way up the page. But, inverting the colors of the logo might not be suitable as I guess a brand would like their main logo consistent in colors. Hence, this proposal to have an option to not invert the logo.

in components/common/Header.js you have your logo and logo-container:

      <div className="logo-container">
        <img
          src={`/icon/${showMobileMenu ? 'cross' : 'menu'}.svg`}
          onClick={this.toggleMobileMenu}
          className="w-32 mr-1 d-block d-sm-none"
          alt="Menu icon"
        />
        <Link href="/">
          <a>
            <img
              src="/images/commerce.svg"
              className="logo cursor-pointer"
              alt="Logo"
            />
          </a>
        </Link>
      </div>

In the same file, there is this code which I believe is responsible for the header invert:

    <div
      ref={this.header}
      className={`d-flex header align-items-center justify-content-between position-relative ${
        transparent ? '' : 'invert'
      }`}
    >

Anyone have suggestions on this fix?

ScopeyNZ commented 4 years ago

Sure. You can just remove this bit:

 ${transparent ? '' : 'invert'}

That is a ternary operator that is adding the invert class if the transparent variable is true. If you never want the invert class, you can just remove that bit. If you want it to always be inverted, you can replace it with just invert. The ${...} bit is just a way of putting some JS code in the middle of the string.

I'll close this issue for now. Thanks for the feedback and good luck with your store!

LB22 commented 4 years ago

Hey. Thanks for the answer @ScopeyNZ . Few more things. Why are anchor tags put inside next link? I've been trying to google it but can't find anything of value. Why not only the Link tag when navigating inside the app? Why the double href?

for example: components\common\Header.js

        <Link href="/collection">
          <a href="/collection" className="mr-4 font-color-black">Shop</a>
        </Link>

        <Link href="/about">
          <a href="/about" className="mr-4 font-color-black">
            About
          </a>
        </Link>

I would also like to ask about the eslint warning for target:"_blank"

This occurs in components\common\Footer.js and pages\about.js

          <a
            href="https://commercejs.com/company/about"
            className="mb-3 d-block font-color-medium"
            target="_blank"
          >
            About
          </a>

Is this something you should fix or how is this warning to be considered?

There is also an accessibility related eslint warning about the anchor tag in components\homepage\CategoryBanner.js

This doesn't worry me so much, but I wanted to point it out for you just in case you need to do something about it.

Should I open these things as a new issue or is this comment enough?