Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breadcrumb changes #46

Merged
merged 4 commits into from
Feb 20, 2017
Merged

Breadcrumb changes #46

merged 4 commits into from
Feb 20, 2017

Conversation

hminnovation
Copy link
Contributor

@hminnovation hminnovation commented Feb 18, 2017

In the sprint I rather hastily created base to be

    <content>
        {% block content-header %}
        {% endblock content-header %}

        {% block breadcrumbs %}
            {% breadcrumbs %}
            {# breadcrumbs is defined in base/templatetags/navigation_tags.py #}
        {% endblock breadcrumbs %}

        {% block content-body %}
        {% endblock content-body %}
    </content>

This allowed breadcrumbs to be nested between the hero image of the design and the main content, but having started properly with the styling it's a brittle solution that's going to be a headache.

I'm moving it back to the more orthodox

{% block breadcrumbs %}
    {% breadcrumbs %}
    {# breadcrumbs is defined in base/templatetags/navigation_tags.py #}
{% endblock breadcrumbs %}

<content>
    {% block content %}
        
    {% endblock content %}
</content>

To do

  • Amending relevant template files
  • Styling breadcrumb
  • Dealing with any merge issues

@shacker
Copy link
Contributor

shacker commented Feb 19, 2017

When I run this branch, all content disappears on the site. Changing the main block from block content to block content-body brings it back, but thumbnail images on locations index go all stretchy. Can you test?

@hminnovation
Copy link
Contributor Author

Sorry, I should have made it clearer that this branch isn't ready to test yet. I just wanted to create some visibility of the fact it was being done :)

@shacker
Copy link
Contributor

shacker commented Feb 19, 2017

Ah - our work team convention is to put a big DO NOT MERGE flag as top comment on WIP branches that also have PRs.

@hminnovation
Copy link
Contributor Author

It's ours too. Sorry, entirely my fault for having done this whilst being rushed out the door by an irate partner who didn't want me working on a Saturday :D

So, I think this is now ready to review. The breadcrumbs should look like
capture d ecran 2017-02-19 a 21 49 03
capture d ecran 2017-02-19 a 21 52 27

(The two images above are with the HTML cherry-picked from branch 51-location-styling-missing-styles because it's otherwise not possible to show the hero image)

I've used a bit of a hack within the CSS .breadcrumb-container + content > .hero to allow the breadcrumb to sit visually above hero images using a negative top-margin, but on pages where there isn't a hero image the breadcrumb will sit above the white background.

There's going to be a lot of conflicts between this and #53 . I would suggest that we merge #53 to master (if no more work required on that PR), then I merge that version of master in to this branch, and resolve the conflicts. Does that sound okay?

@shacker
Copy link
Contributor

shacker commented Feb 19, 2017

LOL, I sure get that about partner not loving this weekend work :)

PR #53 is now merged, so rebase away. This looks really polished - Thanks for all your work on this.

@shacker
Copy link
Contributor

shacker commented Feb 19, 2017

LGTM.

+1 on returning to standard block content.

@hminnovation
Copy link
Contributor Author

Awesome. Have rebased. @shacker or @daaray if one of you could double check and merge that'd be awesome.

@daaray
Copy link
Contributor

daaray commented Feb 20, 2017

👍 :shipit:

@daaray daaray merged commit 09dc151 into master Feb 20, 2017
@daaray daaray deleted the 33-breadcrumb-changes branch February 20, 2017 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants