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

Migrate KAM smartmenus to core #13582

Merged
merged 1 commit into from Feb 25, 2019

Conversation

@colemanw
Copy link
Member

colemanw commented Feb 12, 2019

Overview

Ports the KAM extension into CiviCRM core.

Before

Extension required to get the new smartmenus-based menubar.

After

New smartmenus-based menubar included with core. KAM and Slicknav extensions marked obsolete.

@civibot

This comment has been minimized.

Copy link

civibot bot commented Feb 12, 2019

(Standard links)

@civibot civibot bot added the master label Feb 12, 2019
@colemanw colemanw force-pushed the colemanw:kam branch 2 times, most recently from 21c55c8 to 9d7c842 Feb 12, 2019
@colemanw

This comment has been minimized.

Copy link
Member Author

colemanw commented Feb 12, 2019

The KAM extension has gone through a month's-long period of review & refinement. At this point it's been tested on all the CMSs with a wide variety of settings. I believe it's time to merge it into master so it will be part of next month's RC.

FYI @JoeMurray @aydun @monishdeb @mattwire @vingle @christianwach @themak1985 @laryn @artfulrobot

@colemanw

This comment has been minimized.

Copy link
Member Author

colemanw commented Feb 13, 2019

@civicrm-builder retest this please

@seamuslee001

This comment has been minimized.

Copy link
Contributor

seamuslee001 commented Feb 13, 2019

@colemanw this needs a rebasing now

@colemanw

This comment has been minimized.

Copy link
Member Author

colemanw commented Feb 14, 2019

@civicrm-builder retest this please

@monishdeb

This comment has been minimized.

Copy link
Member

monishdeb commented Feb 14, 2019

Awesome. Will start with D8 and D7 and its accessibility behavior as listed in https://lab.civicrm.org/dev/accessibility/issues/1. I also like to invite others to review this patch, as this is a big improvement towards Civi Menu and patch be should be scrutinized on all aspects. @colemanw should I mention this PR in dev channel?

@monishdeb

This comment has been minimized.

Copy link
Member

monishdeb commented Feb 14, 2019

@colemanw isn't the patch should include the bower.json commit to include the smartmenu library bower_components/smartmenus/dist/jquery.smartmenus.min.js ?

@colemanw colemanw force-pushed the colemanw:kam branch from 73da5d4 to 7062737 Feb 14, 2019
@colemanw

This comment has been minimized.

Copy link
Member Author

colemanw commented Feb 14, 2019

@monishdeb - very strange that when I typed bower install smartmenus it did not update the bower.json file. I've now done it manually.

@seamuslee001

This comment has been minimized.

Copy link
Contributor

seamuslee001 commented Feb 15, 2019

Jenkins re test this please

@colemanw

This comment has been minimized.

Copy link
Member Author

colemanw commented Feb 16, 2019

I think this PR is a bit daunting for one person to want the responsibility of being the one to press the Merge button. But it has received extensive testing as an extension.
I'd like to get it merged into the RC so it can get more eyes on it before the release.
@totten would you be willing to merge this based on a quick code review?

@colemanw colemanw force-pushed the colemanw:kam branch 2 times, most recently from f1c892c to f766a46 Feb 16, 2019
@colemanw colemanw force-pushed the colemanw:kam branch from f766a46 to b30809e Feb 21, 2019
@eileenmcnaughton eileenmcnaughton added this to requires more work to be reviewable in Product maintenance Feb 24, 2019
@eileenmcnaughton eileenmcnaughton added this to needs work to be reviewable in review board Feb 25, 2019
@colemanw

This comment has been minimized.

Copy link
Member Author

colemanw commented Feb 25, 2019

@civicrm-builder retest this please.

@eileenmcnaughton what additional work is needed? FYI I have already rebased in the recent bug fixes from the KAM extension.

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

eileenmcnaughton commented Feb 25, 2019

@colemanw my understanding is that this will be a big problem if we merge this before updating shoreditch - also I would err towards merging straight after the rc is cut to give it max review time

@colemanw

This comment has been minimized.

Copy link
Member Author

colemanw commented Feb 25, 2019

I'm ok with bumping this back one more release & merging next week. But I'm less concerned about Shoreditch as there is already a PR in the works on that side, and this would just be one of several breaking changes to core that have been released lately (another being civicrm/org.civicrm.shoreditch#356)

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

eileenmcnaughton commented Feb 25, 2019

@colemanw ah that one too huh? What is the status with the shoreditch sprint - I feel like a goal of that sprint should be that this is tested with shoreditch & given the 'merge-ready' flag

@mattwire

This comment has been minimized.

Copy link
Contributor

mattwire commented Feb 25, 2019

I feel like a goal of that sprint should be that this is tested with shoreditch & given the 'merge-ready' flag

Kind-of. As @colemanw says there are already a few breaking changes in core as far as the shoreditch extension is concerned and outstanding PRs for the extension. The KAM menu actually works well with shoreditch already (I'm using it on all my production sites) and I don't think anything in the shoreditch extension should be seen as a blocker to this PR being merged - in fact I think this PR needs merging before changes are merged in the shoreditch extension - so the minversion on shoreditch can be bumped to the release containing this PR and we can rip out various bits that support old versions of Civi.

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

eileenmcnaughton commented Feb 25, 2019

@mattwire OK - that's a reasonable case - ie a bunch of work is being done on shoreditch - now-ish? so it would be better if this was merged for that to be done against....

Do you think we should just merge this now & deal with anything that comes out of it?

@mattwire

This comment has been minimized.

Copy link
Contributor

mattwire commented Feb 25, 2019

Do you think we should just merge this now & deal with anything that comes out of it?

I would say so, it's had pretty good user testing as an extension. I'm sure there will be one or two issues that pop up but we're not going to find them until it's merged. And, we've just had a security release so there are likely to be less forced upgrades to the next version (ie. more time for issues to be found).

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

eileenmcnaughton commented Feb 25, 2019

@mattwire OK - I'll take your advice on it - I guess one thing is that this will be well tested as EVERYONE uses the menu - so devs should be giving it a good bash over the next 6 weeks

@eileenmcnaughton eileenmcnaughton merged commit 3291b80 into civicrm:master Feb 25, 2019
1 check passed
1 check passed
default Build finished.
Details
Product maintenance automation moved this from requires more work to be reviewable to Pending Merge Feb 25, 2019
review board automation moved this from needs work to be reviewable to done Feb 25, 2019
@eileenmcnaughton eileenmcnaughton deleted the colemanw:kam branch Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Product maintenance
  
Pending Merge
5 participants
You can’t perform that action at this time.