FIX Re-instate CWP recipe version in help menu

silverstripe / cwp-core

CWP basic compatibility module

BSD 3-Clause "New" or "Revised" License

3 stars 12 forks source link

FIX Re-instate CWP recipe version in help menu #58

Closed robbieaverill closed 5 years ago

robbieaverill commented 5 years ago

This reinstates the missing version provider configuration. I don't know where this went, but it was introduced in CWP 1.9 and should have been in CWP 2.x.

Fixes #57

This is what it looks like now:

Previously the version number wasn't shown until you hovered over the logo, which would then say SilverStripe (Version - CWP: 2.2.x-dev). With the redesigned help menu, it might be misleading to have 2.2 next to SilverStripe. I previously asked whether we should change this to "CWP" for CWP, but we decided not to.

@clarkepaul @sachajudd might need your input here before this gets merged

clarkepaul commented 5 years ago

I really don't think changing the version of SilverStripe is way to go, especially that it sits next to the brand name. The CWP version number includes a wider group of things than the CMS, in other words the CMS is one part of CWP and the fact that the CMS version is still 4.x still holds true (well in my mind).

We could add an additional line specifically for CWP though for those that find the CMS version number miss-leading, for example...

robbieaverill commented 5 years ago

That looks great, I’ll look at it tomorrow

clarkepaul commented 5 years ago

Cool as, don't take that text colour though as it wont pass accessibility (did it as a PoC). Just use the font colour from the word SilverStripe—I used font size 9px but see what works best, you just do what looks right to you as thats something we could tidy up later if we needed to.

robbieaverill commented 5 years ago

We've got a text-align: center on the title already, so it's looking like this without modifying the alignment:

I'll push this up for your review on Monday.

The mouse tooltip over "4.3" reads SilverStripe (Version = CMS: 4.3.x-dev, CWP: 2.2.x-dev)

clarkepaul commented 5 years ago

@robbieaverill, @sachajudd will make a PR to align the text to the left on Admin.

scott1702 commented 5 years ago

The text-align: center you're seeing is a user agent style so not something exclusively specified in admin. It's only due to the additional cms-help__toggle-cwp-title span that causes the centred behaviour so any fixes relating to this should be addressed as part of this modules rather than separately directly into admin. @sachajudd will follow with a fix.

sachajudd commented 5 years ago

Updated PR screenshots:

robbieaverill commented 5 years ago

Thanks! Looks good