refactor header partial to include _main_nav_bar and _nav_bar markup at same place so that easily overriden with deface #1851

Closed
wants to merge 1 commit into
from

Projects

None yet

7 participants

@kunalchaudhari

It is common requirement where people might want to add links from/to nav_bar to/from main_nav_bar. So if they all at same place which header than easily overridden using deface.

@travisbot

This pull request passes (merged 33d96974 into cae1f13).

@travisbot

This pull request passes (merged 54c5c1e into cae1f13).

@joneslee85
Spree Commerce member

@kunalchaudhari do you have any difficulty overring those two partial using Deface?

@kunalchaudhari

Yeah...as i wanted to move the Home and Cart link to nav_bar.....

@kunalchaudhari

So what happens is, i am able to insert whole partial (_main_nav_bar) in nav_bar but mark up is ul#main_nav_bar. So that is not very flexible as it results in dirty html where i have ul#nav_bar and under that ul#main_nav_bar. Rather above commit give flexibility and enable user to easily remove/replace/move individual parts of markup. and if we see this all comes under header and user should be able to move individual parts in header. So keeping all in header partial is good enough with this advantage.

@kunalchaudhari

@joneslee85 i can override those two partials with Deface but when i wanted to move links between those partial than its not possible.

@joneslee85
Spree Commerce member

@kunalchaudhari If you want to move those links from one partial to other, you could remove from one and add to other. Please make sure you set up the ordering for deface overrides so that the removal override happen before the addition override.

@kunalchaudhari

@joneslee85 i want to do header like this.

Header

here is repo with implementation with spree_master and kunalchaudhari_branch (which has above commit).

https://github.com/kunalchaudhari/spree_view_customization_with_deface_issue_1851

Here are the overrides with both branch.

https://github.com/kunalchaudhari/spree_view_customization_with_deface_issue_1851/blob/master/spree_master/app/overrides/move_main_nav_links_to_top_nav.rb

  1. modify _nav_bar partial and move _main_nav_bar partial to _nav_bar partial
  2. now main_nav_bar is under nav_bar so move cart-link from ul#main-nav-bar to ul#nav-bar
  3. now main_nav_bar is under nav_bar so move homet-link from ul#main-nav-bar to ul#nav-bar
  4. finally remove ul#main-nav-bar from ul#nav-bar

Whereas using above commit it is achieved very easily and conveniently. And it only requires two override.

https://github.com/kunalchaudhari/spree_view_customization_with_deface_issue_1851/blob/master/kunalchaudhari_branch/app/overrides/move_main_nav_links_to_top_nav.rb.rb

  1. move store_menu partial to top nav-bar
  2. remove ul#main-nav-bar

As you can see its quite difficult with spree_master to override. take bit of code. I have placed all overrides in one file for simplicity.

@schof
Spree Commerce member

Let's ask @BDQ what he thinks. He's the deface expert ;-)

@JDutil
Spree Commerce member

I like this change since I think it will make working with deface simpler too.

@kunalchaudhari I'd also put the store menu partial in the header. Why create a new partial after removing the first two?

@kunalchaudhari

@jdutil yeah i agree. Let's see what deface expert @BDQ says about this :)

@radar
Spree Commerce member

@BDQ: Any update on this issue?

@BDQ
Spree Commerce member
BDQ commented Oct 8, 2012

The "problem" with view changes like this is that they break existing overrides that are defined against those partials that are being removed.

Personally, I'm all for reducing the number of view / partials that are rendered on each page for two reasons:

1) it makes using Deface simpler when there's less partials / templates in contention.

2) it makes rendering faster, I'm my experience every partial rendered slows down the overall response time for a page.

Recent changes like: #1361 actually did the opposite, and introduced more partials. So before we merge this PR we need to decide which strategy we're actually going to embrace?

IMO, if we do decide to make view changes, where templates are being merged (or split) then these changes can only be released as part of a Major version bump (as our views are effectively part of our API now).

@kunalchaudhari

@BDQ can we do something like moving things between partials? i don't know if deface has this ability. Something like source is partial and target is also some other partial.

@BDQ
Spree Commerce member
BDQ commented Oct 9, 2012

You mean like :cut from one template and :insert_x into another?

If so, not currently but it's something I've been planning to add.

@kunalchaudhari

@BDQ exactly :) Great to have that...

@radar
Spree Commerce member

Closing this issue, as there has been no activity on it in over a month.

@radar radar closed this Nov 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment