ENH Prepare theme for adoption as a supported module #9
Conversation
GuySartorelli
force-pushed
the
pulls/main/adopt-theme
branch
10 times, most recently
from
04794fb to
e350cbe
Compare
| <link rel="manifest" href="$resourceURL('themes/startup/favicons/site.webmanifest')"> | ||
| <link rel="mask-icon" href="$resourceURL('themes/startup/favicons/safari-pinned-tab.svg')" color="#051e2d"> | ||
| <link rel="shortcut icon" href="$resourceURL('themes/startup/favicons/favicon.ico')"> | ||
| <link rel="apple-touch-icon" sizes="180x180" href="$themedResourceURL('favicons/apple-touch-icon.png')"> |
There was a problem hiding this comment.
$themedResourceURL means cascading themes can replace the favicon with something appropriate for a given project
| @@ -6,6 +6,7 @@ | |||
| <div class="page__content"> | |||
| <h1 class="page__title">$Title</h1> | |||
| $Content | |||
| $ElementalArea | |||
There was a problem hiding this comment.
Elemental is used super often - makes sense to include it in the page layout template.
If there's no elemental area, this will do nothing so it does no harm by being included and makes it easier to use this theme (especially by maintainers who can now see the elemental area rendered without having to adjust anything from a fresh install)
| <script type="module" src="{$resourceURL('themes/startup/js/startup.js')}" defer></script> | ||
| <% if $HasPerm('CMS_ACCESS') %>$SilverStripeNavigator<% end_if %> | ||
| <script type="module" src="{$themedResourceURL('js/startup.js')}" defer></script> |
There was a problem hiding this comment.
$themedResourceURL being used to match the <% require themedCSS('startup') %> further up which uses theme cascade.
Note we can't use a <% require %> directive for this js because there's no way to pass in an array, which would be needed to get the type and defer in there.
| @@ -15,6 +15,7 @@ | |||
| <% include Header %> | |||
| $Layout | |||
| <% include Footer %> | |||
| <script type="module" src="{$resourceURL('themes/startup/js/startup.js')}" defer></script> | |||
| <% if $HasPerm('CMS_ACCESS') %>$SilverStripeNavigator<% end_if %> | |||
There was a problem hiding this comment.
Adding $SilverStripeNavigator makes it easier to regression test that functionality, and is useful for authenticated users.
images/block1-image.webp
Outdated
images/chevron--down--white.svg
Outdated
css/accordion.css
Outdated
There was a problem hiding this comment.
Adds functional CSS to correspond with the accordion.js
Previously, CSS was applied only to narrow-scoped navigation-related CSS classes and was not reusable.
This both allows for additional accordions to be added using this JS (e.g. in an accordion elemental block) and allows the same functionality to be used for both mobile nav and desktop nav.
css/base.css
Outdated
| .error.message { | ||
| color: var(--color-error); | ||
| } |
There was a problem hiding this comment.
Form validation messages - test using userforms.
css/header.css
Outdated
There was a problem hiding this comment.
Changes here include:
- Expand link to fill the whole tappable space in mobile nav (used to only be the text itself - tapping slightly to the side would do nothing)
- Remove functional CSS that is now in the accordion styling
- Remove on-hover expanding of desktop menu in favour of reusing existing accordion functionality
- Update some styling for hover states and layout as appropriate to accomodate the above
There was a problem hiding this comment.
I'm a bit confused by this PR, it seems like there's a bunch of changes made from what the Pikachu's originally did and was presumably signed off by the design team? Or have I misunderstood what this PR's doing?
Based on the notes in the original description it seemed like this PR should only be changing the composer name?
|
The work the Pikachus did was primarily for the purposes of the demo. This PR brings the theme up to a standard we'd be comfortable with for the default supported installer theme. These changes come after these acceptance criteria from the linked issue:
In my view, the theme as it was before this PR is not of an appropriate standard, as it is missing some important styling (e.g. table styles, margin below paragraphs, some hover styles), is missing template variables that should be included (e.g. The changes I've made in this PR bring it to the minimum standard I'm comfortable with including as the default supported theme. |
There was a problem hiding this comment.
TinyMCE formatting doesn't work:
- Bullet list
- Numbered list
- Align center
- Align right
- Justify
- Italic displays as underline
Content block vertical spacing
In a content block context, along with the TinyMCE formatting points above, vertical spacing is also lacking when using the out of the box content block
"Home" in the navigation
In the breadcrumb the page "Home" is (incorrectly IMO) acting as the root page where pages like "About us" are treated as child pages, rather than sibling pages. I don't think we should show "Home" in the breadcrumb. You can simply click on the top-left logo to get back to the home page which is intuitive. Also just seeing the word "Home" looks weird, you don't see this on most websites.
Also I don't think "Home" should show in the top/accordian nav, as again you can just click the logo to go to the home page
There was a problem hiding this comment.
Userform vertical spacing
In a userform context, along with the TinyMCE formatting points above, vertical spacing is lacking
Deeply nested pages don't work in accordion
In the accordion nav, sub-sub pages do not show
Excessive vertical space in breadcrumb
There is excessive vertical space between the breadcrumb and the header
|
Discussed offline:
|
|
Turns out most of the typography styling was in the I have now moved that out into the frontend CSS (which is also included in the WYSIWYG field in the CMS).
This is the one thing I cannot reproduce. |
Assuming you mean space between blocks, this seems fine to me? Note that "Okie doke" is the end of one block, and "some block" is the start of the next one. |
Agreed for the most part - except I've kept it there in the case where a child page is explicitly the child of the home page.
That can be configured in the CMS by setting "show in menus" to false. Because of this I won't change how the template handles it unless you insist. |
GuySartorelli
force-pushed
the
pulls/main/adopt-theme
branch
from
e350cbe to
037cd69
Compare
css/block.css
Outdated
There was a problem hiding this comment.
Styles here weren't being used
css/hero.css
Outdated
There was a problem hiding this comment.
Styles here weren't being used
css/section.css
Outdated
There was a problem hiding this comment.
Styles here weren't being used
css/transitions.css
Outdated
There was a problem hiding this comment.
Moved to base to reduce number of network calls required to load all css since this is just a single declaration
css/wysiwyg.css
Outdated
There was a problem hiding this comment.
Moved relevant styles to typography so they apply to frontend as well
| @@ -70,7 +71,6 @@ img { | |||
| display: block; | |||
| height: auto; | |||
| max-width: 100%; | |||
| width: 100%; | |||
There was a problem hiding this comment.
Was overriding the height and width attributes from WYSIWYG
css/form.css
Outdated
There was a problem hiding this comment.
Form styles adapted from simple theme
GuySartorelli
force-pushed
the
pulls/main/adopt-theme
branch
from
037cd69 to
cf44939
Compare
There was a problem hiding this comment.
Opened desktop "accordian" should close if click anywhere on page that's not the accordian
Set h1 margin-bottom: 2.4rem;
Set h2,h3,h4,h5,h6 margin-top: 2.4rem;
The right hand nav that shows on subpages
- parent page link should be underlined to show that it's a link
- on mobile, needs a thin grey horizontal line above it to denote that it's a nav section as moves from the right and will display underneath page content. Should remove the thin grey line below the parent page link on mobile view.
That's not how accordions tend to work - but I'll agree to doing this in the narrow scope of the menu. I'll add code for this that is specific to the menu or to some optional attribute that other accordions can choose to exclude.
Oh, I didn't know that was a link lol. Will do. |
|
Converted the |
GuySartorelli
force-pushed
the
pulls/main/adopt-theme
branch
from
cf44939 to
0990299
Compare
|
Made the requested changes - also noted the JS was missing semicolons so I've plopped those in. |
Important
You have to use the old name
silverstripe-themes/startupas your dependency when testing this PR - composer seems to look at the composer.json in the base branch rather than the PR branch even when installing the PR as a forkIn addition to what's explicitly pointed out in comments below, the following changes were made:
$SilverStripeNavigatorfor logged-in users (shows whether you're viewing draft or live and gives edit button)<base>tag anddirattributeIssue