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

Use the same sidebar logic that Solidus uses #6

Closed
wants to merge 2 commits into from
Closed

Use the same sidebar logic that Solidus uses #6

wants to merge 2 commits into from

Conversation

docelic
Copy link
Contributor

@docelic docelic commented Dec 27, 2016

This change makes the default static_content sidebar use the same logic
that Solidus does in frontend/app/views/spree/products/index.html.erb.

The positive consequence of this change is that now, by default, static
content pages render the same default sidebar that Solidus pages do,
unifying the look and feel and making the static content pages look
"normal".

This change makes the default static_content sidebar use the same logic
that Solidus does in frontend/app/views/spree/products/index.html.erb.

The positive consequence of this change is that now, by default, static
content pages render the same default sidebar that Solidus pages do,
unifying the look and feel and making the static content pages look
"normal".
This commit replaces the Deface hook with a standard MenuItem approach to
add the "Pages" entry into the Admin menu.

Besides removing Deface, this change has an additional benefit that the
Pages entry now behaves in the same way that the original Solidus admin
menu entries do.
(Previously, with Deface, the "Pages" entry was being added to the menu
even on the Admin Login page, where the standard menu should remain empty.)
@docelic
Copy link
Contributor Author

docelic commented Dec 28, 2016

The change in the second commit seems to be incompatible with Solidus < 1.4.

If this is a problem for the pull request to be merged, I could change the code to retain the Deface behavior for < 1.4, and use MenuItem for >= 1.4.

(Advice on whether that's a good approach, and/or what to do instead would be appreciated.)

Thanks!

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks and sorry for the belated answer. Could you please remove the second commit as this is superseded by #10 now.

@jtapia
Copy link
Contributor

jtapia commented Mar 8, 2018

@tvdeyen #16

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