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

[HOLD] 🎨 Split asset helper for admin & theme #8105

Closed
wants to merge 1 commit into from

Conversation

ErisDS
Copy link
Member

@ErisDS ErisDS commented Mar 7, 2017

requires #8126

This PR takes the {{asset}} helper, which is currently one function shared between admin and themes, and splits it into two functions / helpers, one for admin and one for themes.

By dividing it up into two different wrappers for the getAssetUrl() function, we can simplify the logic in each function.

This removes the dependency between /admin and /helpers, so that we can reasonably move the theme helpers into /themes to group all of that logic together. We can also then simplify the loading of helpers for themes, much as I have done in this PR for admin helpers.

We can also remove the minifyAssets config, because the case for converting path names from .js to .min.js is explicitly only for the admin panel in production. We control this logic, and therefore it should not be exposed as config. In this particular case, I think it is correct to use config.get('env'). Eventually, we hope to get ember to figure this out somehow.


refs #7488, #7491

  • At the moment, we use the same asset helper function for both admin and theme
  • Both functions are (rightly or wrongly) a wrapper around meta.getAssetUrl()
  • The usecases in these contexts are v. different:
    • asset for the admin is used only in the generated index.html file
    • ghost="true" is always true in this case, except for the favicon which is ALREADY a separate case
    • minifyInProduction is only used to the admin panel, not documented or supposed to be used for themes
  • For themes, the helper doesn't take any arguments, it should just call to getAssetUrl()
    If we do want to introduce some sort of .min path adjuster we should give it a better name!
  • For the admin panel, minifyInProduction means exactly what it says - minify if env === production,
    it makes no sense IMO to move this to config because we control it and also plus "minifyAssets" made it
    sound like we had an asset pipeline when all this did was change the output from .js to .min.js or .css to .min.css

refs TryGhost#7488, TryGhost#7491

- At the moment, we use the same asset helper function for both admin and theme
- Both functions are (rightly or wrongly) a wrapper around meta.getAssetUrl()
- The usecases in these contexts are v. different:
    - asset for the admin is used only in the generated index.html file
    - ghost="true" is always true in this case, except for the favicon which is ALREADY a separate case
    - minifyInProduction is only used to the admin panel, not documented or supposed to be used for themes
- For themes, the helper doesn't take any arguments, it should just call to getAssetUrl()
  If we do want to introduce some sort of .min path adjuster we should give it a better name!
- For the admin panel, minifyInProduction means exactly what it says - minify if env === production,
  it makes no sense IMO to move this to config because we control it and also plus "minifyAssets" made it
  sound like we had an asset pipeline when all this did was change the output from .js to .min.js or .css to .min.css
@kirrg001
Copy link
Contributor

kirrg001 commented Mar 9, 2017

Eventually, we hope to get ember to figure this out somehow.

Could you please explain this to me?Maybe worth adding it to the comment?

Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍

@ErisDS
Copy link
Member Author

ErisDS commented Mar 9, 2017

Eventually, we hope to get ember to figure this out somehow.

Could you please explain this to me?Maybe worth adding it to the comment?

Ember has its own concept of asset hashing that we currently circumvent. Our usecase is a bit weird, because we don't know at build time what the url for assets will be because of subdirectories. That is why we use a server-side helper & copy index.html -> default.hbs inside of the server. At somepoint we hope to figure out a more-embery way to do it so that we don't have to circumvent ember's asset hashing anymore and use this crazy workaround.

As far as I know there is no plan or idea for how this will work, we're just working on minimising the number of workarounds we have when we can. cc @kevinansfield

@ErisDS ErisDS changed the title 🎨 Split asset helper for admin & theme [HOLD] 🎨 Split asset helper for admin & theme Mar 9, 2017
@ErisDS
Copy link
Member Author

ErisDS commented Mar 9, 2017

😞 I've realised this leaves us with a problem.

The default user-error.hbs (should be called error...), private.hbs & subscribe.hbs templates all make use of the ability to include the css & several image assets from Ghost admin and uses both the ghost and minifyInProduction attributes.

This is related to #6864

Therefore I need to revisit this fix.

@JohnONolan is this setup, where the default templates for errors, private blogging & subscribers are using the admin styles and icons the desired setup? Or would it in anyway be a thing to have a smaller set of specific assets for these defaults? Especially considering #8107 means the icon font is going away - how do you imagine this working with these server-side rendered pages that don't have access to the {{inline-svg}} helper?

@ErisDS
Copy link
Member Author

ErisDS commented Mar 9, 2017

To provide a bit more context, here are the 3 templates that are currently sharing styles with the admin panel:



The stuff they depend on is....

Subscribe template:

  • core/built/assets/ghost.css / ghost.min.css
  • back arrow icon

Private template:

  • core/built/assets/ghost.css / ghost.min.css
  • lock icon

Error template:

  • core/built/assets/ghost.css / ghost.min.css
  • /core/client/public/assets/img/404-ghost.png
  • /core/client/public/assets/img/[email protected]

@ErisDS
Copy link
Member Author

ErisDS commented Mar 10, 2017

OK so I have a 👍 from @JohnONolan on the principle of splitting the styles out here and removing the dependency between the theme and admin layer.

This means we need to do the work of splitting up the CSS if we decide to merge this.

@ErisDS
Copy link
Member Author

ErisDS commented Mar 14, 2017

Closing this for now. Once #8126 is done, we will need to do a slightly different version of this which simplifies the asset helper for themes.

For now we just need to remove the concept of the admin asset helper, this is cleanup from the admin split and can be done separately.

@ErisDS ErisDS closed this Mar 14, 2017
@ErisDS ErisDS deleted the split-asset-helpers branch September 8, 2017 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants