NEW Add CMS community help menu to cms-menu #615
Conversation
f07adad to
b744493
Compare
|
I like the concept. Not sure about the lack of a border for that bottom help panel and the "fade-out" effect on the LeftAndMain links. Might need to discuss what links to include in there. I wouldn't include the "developer doc" link for example, as I would expect CMS user to be the primary target of this help. Some partners might object to us advertising our self too much in the CMS. Maybe something like "report a bug" might be nice. I like the version tag, that probably will be helpful when talking to client to figure exactly which version they are using. |
|
These links help to bring awareness of the wider community as a whole. The user help guides are fairly sparse compared to the developer docs and we found that some CMS users were referring to the tech docs as they go into the configuration of modules a bit more (people eg. POs wanted to know what discussions need to be had with devs). Its also often that there isn't any user help for modules, but there is tech docs. I had the idea for 4 initial links but the "feedback" link might be able to double with the "Bug" idea, essentially they are trying to capture the same type of feedback. As for the gradient, thats a minor detail we can finesse if it doesn't look quite right, its easier to make a decision by using the browser rather than through designs. |
|
Would we like to make the links configurable? That would make it easy for agencies to remove links or add their own helpdesk link. Could also allow third party module to insert doc links specific to their module. |
Yep totally - CWP replaces help links with its own website, good use case |
|
I have already done this for the help link. @clarkepaul mentioned the whole help menu could be configurable or removed also. |
80cecd4 to
3e92a29
Compare
|
I've updated the UI and made the help links configurable, let me know what you think @clarkepaul @maxime-rainville @robbieaverill 🙂 |
3e92a29 to
b55b6a6
Compare
fba0cd7 to
671d84a
Compare
client/src/components/Menu/Menu.scss
Outdated
| .cms-sitename__link { | ||
| color: $color-brand; | ||
| font-size: 22px; |
There was a problem hiding this comment.
You could use $h1-font-size here
client/src/components/Menu/Menu.scss
Outdated
| justify-content: flex-end; | ||
| border-top: 1px solid rgba($border-color, .5); | ||
|
|
||
| @media (min-width: 476px) and (max-width: 767px) { |
There was a problem hiding this comment.
It'd be best to use Bootstrap mixins for this, e.g. line 167 of this file:
@include media-breakpoint-down(sm) {
// ...
client/src/components/Menu/Menu.scss
Outdated
|
|
||
| @media (min-width: 476px) and (max-width: 767px) { | ||
| .cms-help { | ||
| &__link { |
There was a problem hiding this comment.
Looks BEMmy enough for it not to need to be nested under anything except the media query - thoughts?
client/src/components/Menu/Menu.scss
Outdated
| box-shadow: inset -1px 0 0 #ced5e1; | ||
|
|
||
| &--show { | ||
| .cms-help__menu { |
There was a problem hiding this comment.
This kind of goes against the point of BEM. I guess it'd be better perhaps to apply the modifiers to this element as needed so it doesn't have to be nested this deep.
client/src/components/Menu/Menu.scss
Outdated
| display: flex; | ||
| align-items: center; | ||
| background-color: transparent; | ||
| padding: 13px 12px; |
There was a problem hiding this comment.
Any way this could be made relative to $panel-padding-x or $panel-padding-y? There's some CMS menu variables in _variables.scss that reference them already - would be great to keep it consistent if possible:
// CMS menu
$cms-menu-width: $panel-padding-y * 9;
$cms-menu-width-collapsed: $cms-panel-xs;
There was a problem hiding this comment.
If we were doing it relative based on $panel-padding then it would look like this:
padding: $panel-padding-y - .5385rem $panel-padding-x - 0.6155rem;
Do you think we should stick to the px values as it's cleaner?
There was a problem hiding this comment.
Take a look at $spacer-sm, which I think equates to 12px (could also do: $panel-padding-x / 1.67). For the 13px its a pretty unique number so that the entire linkable area comes to a 52px height, might be able to use 1rem though?
code/LeftAndMain.php
Outdated
|
|
||
| foreach ($helpLinks as $key => $value) { | ||
| $formattedLinks[] = [ | ||
| 'Title' => $key, |
There was a problem hiding this comment.
This title isn't translatable at this point - it'll need to be e.g. $translationKey = str_replace(' ', '', $key) to remove spaces from the string so it can be used as an i18n key, then _t(__CLASS . '.' . $translationKey, $key); to make it translatable
| @@ -1,5 +1,27 @@ | |||
| <div class="cms-help__toggle"> | |||
| <button class="cms-help__menu" type="button"> | |||
| <span class="cms-help__logo font-icon-silverstripe"></span> | |||
There was a problem hiding this comment.
Please use spaces for indentation. There's an editorconfig settings file in the module, it's probably worth adding an EditorConfig plugin to your IDE so this can be honoured automatically =)
| </span> | ||
| <span class="cms-help__badge badge badge-info"> | ||
| <% if $MinorCMSVersion %> | ||
| <span class="cms-sitename__version" title="$ApplicationName (Version - $CMSVersion)">$MinorCMSVersion</span> |
There was a problem hiding this comment.
Pre-existing I realise, but Version - {version} should be a translatable string
671d84a to
ac1a110
Compare
b73534e to
3657da3
Compare
tests/php/LeftAndMainTest.php
Outdated
| */ | ||
| public function testGetHelpLinks() | ||
| { | ||
| Config::inst()->set(LeftAndMain::class, 'help_links', [ |
There was a problem hiding this comment.
- Config::inst()->set(
+ Config::modify()->set(
tests/php/LeftAndMainTest.php
Outdated
| $silverstripeLink = $helpLinks->first(); | ||
|
|
||
| $this->assertEquals($silverstripeLink['Title'], 'SilverStripe'); | ||
| $this->assertEquals($silverstripeLink['URL'], 'www.silverstripe.org'); |
There was a problem hiding this comment.
Can you please flip the arguments? The expected result should be the first argument
3657da3 to
194fbef
Compare
194fbef to
9c63cf5
Compare
This commit removes the "Help" menu item and adds a userhelp, docs, community and feedback help menu to the south toolbar. When the cms-menu is closed the SS logo shows in the cms-menu header and the new help menu is removed. Includes changes to the cms version badge and content that is displayed. The help links are also configurable making it easy for agencies to remove or add their own. Also addresses a fix for the mobile menu toggle functionality. Co-authored by: Scott Hutchinson https://github.com/scott1702
9c63cf5 to
965ecdc
Compare
His feedback has been addressed.
|
Nice :) |
|
Cheers, thanks team! 😄 |
|
Nice work @sachajudd @scott1702! Follow up comments:
I'll raise an issue for the two CWP tasks separately once you've responded =) |
|
Cool, CWP ticket logged as silverstripe/cwp-core#50 |
|
Thanks for considering accessibility on this (it's keyboard accessible). I've raised a ticket to improve the screen reader abilities: #714 |
This commit removes the "Help" menu item and adds a userhelp, docs, community and feedback help menu to the south toolbar.
When the cms-menu is closed the SS logo shows in the cms-menu header and the new help menu is removed.
Includes changes to the cms version badge and content that is displayed.
The help links are also configurable making it easy for agencies to remove or add their own.
Also addresses a fix for the mobile menu toggle functionality.
Co-authored by: Scott Hutchinson https://github.com/scott1702
Resolves: #500
To do: Update badge-info in separate PR.
Update https://userhelp.silverstripe.org/en/4/managing_your_website/overview/#help user help.
Feedback would be much appreciated 🙂
Desktop:
cc @clarkepaul