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

Split out the CSS for default templates #8126

Closed
ErisDS opened this issue Mar 10, 2017 · 11 comments · Fixed by #8234
Closed

Split out the CSS for default templates #8126

ErisDS opened this issue Mar 10, 2017 · 11 comments · Fixed by #8234
Assignees
Labels
good first issue [triage] Start here if you've never contributed before. help wanted [triage] Ideal issues for contributors to help with

Comments

@ErisDS
Copy link
Member

ErisDS commented Mar 10, 2017

This issue comes from PR #8105 and is also related to TryGhost/Admin#574.

In the Ghost server-side code we have 3 "default" templates, which provide some basic styles for a special page, and are allowed to be overridden by a theme.

At the moment, all 3 of these default templates get their styles by importing the ghost.css stylesheet from Ghost-Admin. However, we're trying very hard to separate out the server-side theme part of Ghost and the admin panel, and so we'd like those templates to have their own styles, which can be loaded separately.

To make it possible for us to not load the admin css for these templates, the CSS required to display these templates needs to be split out into a separate file, which can live in shared/ for now.


The 3 templates are:

1. The Error page: core/server/views/user-error.hbs

2. The "private" blog login page:

3. Subscribe to this blog page: core/server/apps/subscribers/lib/views/subscribe.hbs

@ErisDS ErisDS added good first issue [triage] Start here if you've never contributed before. css help wanted [triage] Ideal issues for contributors to help with labels Mar 10, 2017
@TienSFU25
Copy link

I'm a beginner looking to contribute. it will take a while for me to ramp up on Ember and Ghost, but can I sign up for this one anyway?

@ErisDS
Copy link
Member Author

ErisDS commented Mar 11, 2017

@TienSFU25 it would be great to have someone look at this - although please be aware we will need it within the next week or two. You shouldn't need to learn ember as the templates that need the styling separated are all rendered server-side. If you need a hand with anything, we all hang out in the #dev channel on our slack.

@TienSFU25
Copy link

alright, thanks!

@TienSFU25
Copy link

how do I make a new asset file that will be built and served under built/assets? I see the css files in client/app/styles but don't understand how they get bundled and end up in built.

@ErisDS
Copy link
Member Author

ErisDS commented Mar 18, 2017

@TienSFU25 The /built/assets folder is the output folder for the client-side code. The purpose of this issue is to move the CSS needed for these templates out of the client side (i.e. the admin panel code), into the server-side (i.e. the blog code). The new CSS file should use plain CSS & live in /shared.

@TienSFU25
Copy link

I don't have write access to the repo. Can you help with this?

@kevinansfield
Copy link
Member

As noted in #8187, the default 404 page is also pulling the ghost-404.png image from the client built files - I think we should look at copying that too so that any front-end resources are kept in Ghost?

I don't have write access to the repo. Can you help with this?

@TienSFU25 you don't need write access to this repo, you should fork the project, make your changes on a branch and push to your fork then open a Pull Request https://help.github.com/articles/creating-a-pull-request-from-a-fork/ - If you need any help swing by our Slack channel

@kevinansfield kevinansfield added this to the 1.0.0 Beta Ready milestone Mar 22, 2017
@ErisDS
Copy link
Member Author

ErisDS commented Mar 22, 2017

@TienSFU25 have you got some code for us? This change is needed before we can make others

@TienSFU25
Copy link

Yes, I will create a pr before the end of today.

TienSFU25 pushed a commit to TienSFU25/Ghost that referenced this issue Mar 23, 2017
See Issue TryGhost#8126

Adds these files under /shared
- normalizer.css
- error.css
- extracted.css (for subscribers.css and private.css)

Also makes these files available as public static content
@TienSFU25
Copy link

So I made 3 new CSS files, put them under /shared, and used serveSharedFile under blog/app. Then I had the .hbs files point to these CSS.

If these three template files are the only ones that need to be migrated from the client side then this should be the correct subset of CSS required. If there are more template files, I think splitting these CSS files into things like layout.css, text.css might make more sense?

If there's a better, more "Ghosty" way to do this let me know.
Also this PR doesn't handle images (there are 2 icons missing). Should I make a new issue for that?

TienSFU25 pushed a commit to TienSFU25/Ghost that referenced this issue Mar 25, 2017
See Issue TryGhost#8126

Adds these files under /shared
- normalizer.css
- ghost.css: subset of client side ghost.css

Also makes these files available as public static content
@aileen
Copy link
Member

aileen commented Mar 27, 2017

Also this PR doesn't handle images (there are 2 icons missing). Should I make a new issue for that?

Thanks for pointing that out! I will update the existing #8107 to include the new svg icons in those templates as well!

aileen pushed a commit to aileen/Ghost that referenced this issue Apr 4, 2017
See Issue TryGhost#8126

Adds these files under /shared
- normalizer.css
- error.css
- extracted.css (for subscribers.css and private.css)

Also makes these files available as public static content
aileen added a commit to aileen/Ghost that referenced this issue Apr 4, 2017
closes TryGhost#8126
needs e3acd3c

This is a replacement PR of TryGhost#8217 (thanks @TienSFU25 for the whole work 🤗), because these changes are needed urgently and blocking other work.

Adds a new `ghost.css` file in `/core/shared/` to be used for server side template rendering (`error.hbs`, `subscribe.hbs` and `private.hbs`).
ErisDS pushed a commit that referenced this issue Apr 4, 2017
closes #8126

* Remove default template dependency on client side CSS

See Issue #8126

Adds these files under /shared
- normalizer.css
- error.css
- extracted.css (for subscribers.css and private.css)

Also makes these files available as public static content

* Remove default template dependency on client CSS

closes #8126
needs e3acd3c

This is a replacement PR of #8217 (thanks @TienSFU25 for the whole work 🤗), because these changes are needed urgently and blocking other work.

Adds a new `ghost.css` file in `/core/shared/` to be used for server side template rendering (`error.hbs`, `subscribe.hbs` and `private.hbs`).
ErisDS added a commit to ErisDS/Ghost that referenced this issue Apr 4, 2017
refs TryGhost#8126, TryGhost#8221

- we now have theme/engine instead of requiring express-hbs everywhere
- only error-handler still also requires express-hbs, this is so that we can render errors without extra crud
- TODO: remove the asset helper after TryGhost#8126 IF it is not needed, or else remove the TODO
kirrg001 pushed a commit that referenced this issue Apr 4, 2017
refs #8126, #8221, #8223

✨ New 'Proxy' for all helper requires
- this is not currently enforced, but could be, much like apps
- the proxy object is HUGE
- changed date to use SafeString, this should have been there anyway
- use the proxy for all helpers, including those in apps 😁

✨ 🎨 Single instance of hbs for theme + for errors
- we now have theme/engine instead of requiring express-hbs everywhere
- only error-handler still also requires express-hbs, this is so that we can render errors without extra crud
- TODO: remove the asset helper after #8126 IF it is not needed, or else remove the TODO

🎨 Cleanup visibility utils
🎨 Clean up the proxy a little bit
🚨 Unskip test as it now works!
🎨 Minor amends as per comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue [triage] Start here if you've never contributed before. help wanted [triage] Ideal issues for contributors to help with
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@kevinansfield @ErisDS @aileen @TienSFU25 and others