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

Remove all menubar css and js #342

Closed
colemanw opened this issue Dec 12, 2018 · 14 comments
Closed

Remove all menubar css and js #342

colemanw opened this issue Dec 12, 2018 · 14 comments

Comments

@colemanw
Copy link
Member

The soon-to-be-in-core KAM extension natively implements basically all the things that Shoreditch was adding onto the menubar - lighter color scheme, robust markup, larger size, etc.

I'd recommend we remove all menubar-related css and JS from Shoreditch and start from scratch if there are any remaining tweaks to be done.

@monishdeb
Copy link
Member

I agree. I faced an issue with q-search icon which is specific to shoreditch custom css changes, after KAM extension is installed:
screen shot 2018-12-12 at 2 29 30 pm

I fixed it by removing the css styling for q-search, see here

After fix:
screen shot 2018-12-12 at 2 43 06 pm

@AkA84
Copy link
Contributor

AkA84 commented Dec 17, 2018

@colemanw do you know, approximately, when the KAM extension will be merged into core? It would be best to remove anything menu-related (or to heavily amend it at least) only after the extension has become part of core

@colemanw
Copy link
Member Author

@AkA84 probably next month, which would set the version at 5.11.
But cleaning up the css/js in this extension and testing it with KAM could happen now. It could even be released now, with KAM declared as a dependency. Once KAM is merged with core that dependency would be automatically resolved, based on some work I'm doing right now on the extension management system.

@colemanw
Copy link
Member Author

@jamienovick @AkA84 The inclusion of KAM in core is looming closer with aydun/uk.squiffle.kam#16 wrapping up. I strongly suggest you get someone to test out that extension with Shoreditch in the next week or so in case there is something critical missing from KAM you'd like to see. Pushing changes into that extension is fairly easy while it remains an extension...

@AkA84
Copy link
Contributor

AkA84 commented Dec 21, 2018

@colemanw thanks for the heads-up! The ~1w window though is a bit unfortunate given the proximity to the holidays, so I'm not sure that all the necessary (extensive testing, tweaking of the theme to properly add its own look-and-feel to the menu, etc) can be done before I'm back. For consideration: this could be attacked as part of the community efforts (see https://lab.civicrm.org/extensions/shoreditch/issues/1)

(cc @jamienovick )

@colemanw
Copy link
Member Author

colemanw commented Jan 2, 2019

@AkA84 @jamienovick the holidays slowed me down as well so this is getting pushed to 5.12. However several extensions including CiviTutorial and RecentMenu now declare KAM as a dependency (when KAM is merged into core that dependency will automatically be resolved). I think the time is now for ShoreDitch to do the same.

@colemanw
Copy link
Member Author

civicrm/civicrm-core#13582 has been opened, targeting 5.12 for inclusion in core.

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Feb 20, 2019

I just had multiple bugs reported when we enabled KAM that turned out to be related to this.

@eileenmcnaughton
Copy link
Contributor

I have the 5.13 rc on our staging & locally & it just started to look really bad - I'm gonna try the patch above

Screen Shot 2019-04-13 at 1 29 49 PM

@twomice
Copy link

twomice commented May 22, 2019

FYI, the display issue mentioned by @eileenmcnaughton above is resolved by removing the background-color rule for #civicrm-menu (see here)

@eileenmcnaughton
Copy link
Contributor

Cool - I've been adding css through a hook to override shoreditch where it makes me sad - maybe I should include that

@themak1985
Copy link

@twomice how are you implementing this with alpha31?

@twomice
Copy link

twomice commented Jun 21, 2019

@themak1985 I haven't had to face that yet.

@jamienovick
Copy link

Closing as fixed in latest version

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 a pull request may close this issue.

7 participants