Feature-wc-store-notice

[sean-au]

No description provided.

[sean-au]

turns off default store notice, which is not bootstrap, or compadible with bootstrap
add a new store notice function, only if enabled and notice string not empty
changes the navbar to sticky rather than fixed
removes a div between header and nav, easier to target
adds a close button, and removes :before icon from store notice.

NOTE: the child theme will override these changes - need to copy and paste woocommerce header into child theme to test

[crftwrk]

Thank you for this PR. But we should not do this that way, because the store notice is a temporary thing and touching the header.php is sensitive. We can use the hook <?php bs_after_primary(); ?> for that and add some additional CSS for positioning. Is this an idea?

[crftwrk]

We can use body class .woocommerce-demo-store .fixed-top to push content down like here https://github.com/bootscore/bootscore/blob/main/scss/bootscore/_admin_bar.scss.

[sean-au]

Great I'll look into it I'm just remote for a few days but I'm will get onto the PRs eventually

…

On Fri, 7 Oct 2022, 5:35 am Basti, ***@***.***> wrote: We can use body class .woocommerce-demo-store .fixed-top to push content down like here https://github.com/bootscore/bootscore/blob/main/scss/bootscore/_admin_bar.scss . — Reply to this email directly, view it on GitHub <#280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARTWGA6XBTWDLH42DZ6JGGLWB74H7ANCNFSM6AAAAAAQ6GNB5Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[sean-au]

Hi @crftwrk, I have had a good look into this, here are the options (they all work but have limitations)

1. Original idea - inject with wp_body_open, change navbar to sticky-top, notice has a close button so can be removed if in the way. The sticky-top navbar is pushed down by the injected HTML, but goes back to the top of the page if the notice is closed.
2. Inject with bs_after_primary, or wp_footer (default wc behaviour)
   a) Display with fixed-top, inject an extra body class and push fixed elements and off canvas down cannot have close button
   b) Display with fixed-bottom
   i) has a close button, gets in the way of the footer/body content
   ii) has a close button, add padding to footer equal to height of store notice. When store notice is closed, this padding is still present in the footer. default WooCommerce behavior in store front theme
   iii) No close button, gets in the way of the footer/body content, although the footer, and off canvas elements could be pushed up. Can have a close button, but not if your pushing items up with CSS

I think for UX considerations a close button is important in case the notice is in the way

[crftwrk]

I completely agree that alert must have a close button. What's about this:

We can create a new hook inside the fixed-top above the navbar

  <?php wp_body_open(); ?>

  <div id="page" class="site">

    <header id="masthead" class="site-header">

      <div class="fixed-top bg-light">

        <!-- Something like this -->
        <?php bs_top_nav(); ?>

        <nav id="nav-main" class="navbar navbar-expand-lg">

This is the same result as your first commit, but keeps the fixed-top-class. BTW this hook can be used for any other stuff as well. So, why not?

[sean-au]

Sounds great I'll give it a go

…

On Sat, 8 Oct 2022, 10:41 am Basti, ***@***.***> wrote: I completely agree that alert must have a close button. What's about this: We can create a new hook inside the fixed-top above the navbar <?php wp_body_open(); ?> <div id="page" class="site"> <header id="masthead" class="site-header"> <div class="fixed-top bg-light"> <!-- Something like this --> <?php bs_top_nav(); ?> <nav id="nav-main" class="navbar navbar-expand-lg"> This is the same result as your first commit, but keeps the fixed-top-class. BTW this hook can be used for any other stuff as well. So, why not? — Reply to this email directly, view it on GitHub <#280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARTWGAZGH74JLGKNMJ4I3K3WCGI25ANCNFSM6AAAAAAQ6GNB5Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[crftwrk]

Maybe I have a new idea. We had a php function for warníng if user browsed site by Internet Explorer. We replaced this function by JavaScript for several reasons #37. But function hooked below the </header>

  </header><!-- #masthead -->

  <?php bootscore_ie_alert(); ?>

https://github.com/bootscore/bootscore/blob/652bc3c411b154bf96361b32db689b99f9e9b295/header-woocommerce.php, please scroll down to the bottom. The result was this:

We can use this position for the shop notice. Then we have no trouble with navbar classes or overlapping content. What do you think?

[sean-au]

Great,Is the hook still there, and should I rename it? If yes, what should I rename to?

…

On Sat, 8 Oct 2022, 12:57 pm Basti, ***@***.***> wrote: Maybe I have a new idea. We had a php function for warníng if user browsed site by Internet Explorer. Now we replaced this function by Javascript. But function hooked below the </header> </header><!-- #masthead --> <?php bootscore_ie_alert(); ?> https://github.com/bootscore/bootscore/blob/652bc3c411b154bf96361b32db689b99f9e9b295/header-woocommerce.php, please scroll down to the bottom. The result was this: <https://camo.githubusercontent.com/42905c708cc42224f8a9fe3e8da528d94798f946195d244ed413af3ffd4bace9/68747470733a2f2f626f6f7473636f72652e6d652f77702d636f6e74656e742f75706c6f6164732f323032322f30362f626f6f7473636f72652d69652d7761726e696e672d616c6572742e77656270> We can use this position for the shop notice. Then we have no trouble with navbar classes or overlapping content. — Reply to this email directly, view it on GitHub <#280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARTWGAYLVITQX44YUX2PNPDWCGYYXANCNFSM6AAAAAAQ6GNB5Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[crftwrk]

No, hook is removed. We have to create a new one, but it's simple and upgrade-safe.

header.php

    </header><!-- #masthead -->
    
    <?php bs_after_masthead(); ?>

functions.php

// Hook after #masthead
function bs_after_masthead() {
  do_action('bs_after_masthead');
}

Basic usage in child's functions.php

function my_function() {
  echo '<div class="container mt-5"><p class="mt-4 alert alert-info">Hello Hook</p></div>';
}
add_action('bs_after_masthead', 'my_function', 5);

Result

Now we can use this hook for store notice. What do you think?

[sean-au]

Looks great, thanks for the help, I'll have a go at it tonight.

…

On Sun, 9 Oct 2022, 4:10 am Basti, ***@***.***> wrote: No, hook is removed. We have to create a new one, but it's simple and upgrade-safe. header.php </header><!-- #masthead --> <?php bs_after_masthead(); ?> functions.php // Hook after #mastheadfunction bs_after_masthead() { do_action('bs_after_masthead'); } Basic usage in child's functions.php function my_function() { echo '<div class="container mt-5"><p class="mt-4 alert alert-info">Hello Hook</p></div>'; } add_action('bs_after_masthead', 'my_function', 5); Result [image: screenshot] <https://user-images.githubusercontent.com/51531217/194745261-05eeb796-a7e2-442f-ba04-18388e707a0d.png> Now we can use this hook for store notice. What do you think? — Reply to this email directly, view it on GitHub <#280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARTWGAYDUM5CIE5DPI4TBXDWCJ4WPANCNFSM6AAAAAAQ6GNB5Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[sean-au]

Can you have a look? I had it working, but then I am now having issues with my WP development environment.

[crftwrk]

It is not working on my site. However, this PR has already conflicts with other (merged) PR's, there are 16 files changed. I suggest to close this PR to not mess up here anything and then start with two fresh PR's. One for the hook and the second one for the store notice. We will release 5.2.2.0 this weekend when Justin is ready to review. I would be happy if we can ship at least the hook by this release. Do you agree?

[justinkruit]

@crftwrk should I wait with the release for this PR?

[crftwrk]

@justinkruit let's release now and target this PR to next minor release. Bootstrap 5.2.2 has some very useful bugfixes.

[sean-au]

I'll have a go at rebase after the release

…

On Sat, 15 Oct 2022, 3:22 am Basti, ***@***.***> wrote: @justinkruit <https://github.com/justinkruit> let's release now and target this PR to next minor release. Bootstrap 5.2.2 has some very useful bugfixes. — Reply to this email directly, view it on GitHub <#280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARTWGA6FZBPC6VUDO5KZG4DWDJLRRANCNFSM6AAAAAAQ6GNB5Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>