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

Structural fixes #163

Merged
merged 11 commits into from Oct 11, 2018

Conversation

@tmassman
Copy link
Member

tmassman commented Oct 2, 2018

This PR fixes the current content column and footer implementation.

The current Footer is build on top of footer portlets. But adding additional portlets to the footer will result in a messy and unstyled footer:

footer before

With some XSLT and a little bit of CSS we are able to create a doormat like footer with up to 4 columns (1 portlet: 12 col width 2 portlets: 6 cols, 3 portlets: 4 cols, 4 portlets: 3 cols and more than 4 portlets: 3 cols each), based on the underlying bootstrap grid:

footer with doormat

The content columns are currently in the "wrong" order in the HTML: column 1 comes before the main content column, which makes it hard to style for mobile views (portlet columns (sidebars) should come at the very end). Bootstrap gives us the option to move columns around, but the order needs to be changed to be able to have all sidebar columns below the main content on mobile width screens.

If both portlet columns are available, they will be shown next to each-other on small screens, and stacked on top of each-other on smallest screens:
3 columns small

3 columns smallest

On desktop wide screens, columns will be shown in the classic 3-column layout:
3 columns desktop

tmassman added some commits Oct 2, 2018

Merge pull request #161 from plone/doormat-footer
Add doormat like footer
Change order of content columns.
Have content container before column1 before column2.

@tmassman tmassman requested review from mauritsvanrees , petschki , pbauer and jensens Oct 2, 2018

@tmassman tmassman self-assigned this Oct 2, 2018

@tmassman

This comment has been minimized.

Copy link
Member Author

tmassman commented Oct 2, 2018

Part of #160 .

@mauritsvanrees
Copy link
Member

mauritsvanrees left a comment

I don't usually do theming stuff in our company, so I can't really comment on details. But I like the solution that you describe. Makes sense.

@jensens

This comment has been minimized.

Copy link
Member

jensens commented Oct 3, 2018

this is planned for 5.2 only right? so we need to branch away for 5.1.

@tmassman

This comment has been minimized.

Copy link
Member Author

tmassman commented Oct 3, 2018

Those changes are safe for 5.1.x and 5.2.x (even 5.0.x).

@MrTango MrTango self-requested a review Oct 3, 2018

@MrTango

MrTango approved these changes Oct 3, 2018

Copy link
Contributor

MrTango left a comment

Looks good to me in 5.1/5.2

@jensens

jensens approved these changes Oct 3, 2018

Copy link
Member

jensens left a comment

Love that, merge if ready and tests are green!

@petschki
Copy link
Member

petschki left a comment

great. LGTM!

@tmassman

This comment has been minimized.

Copy link
Member Author

tmassman commented Oct 5, 2018

@MrTango could you please take a look at the failing 5.2 tests?

@pbauer pbauer merged commit d75853a into master Oct 11, 2018

4 checks passed

Changelog verifier Entry found
Details
Plone Contributors Agreement verifier All users have signed it
Details
Plone Jenkins CI - pull-request-5.1 Job finished with success status
Details
Plone Jenkins CI - pull-request-5.2 Job finished with success status
Details

@tmassman tmassman referenced this pull request Oct 18, 2018

Merged

Update footer rules #166

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.