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

Server-side skin building issues #7266

Closed
phit opened this issue Jan 19, 2019 · 17 comments
Closed

Server-side skin building issues #7266

phit opened this issue Jan 19, 2019 · 17 comments

Comments

@phit
Copy link
Contributor

phit commented Jan 19, 2019

  • NodeBB version: 1.11.1
  • NodeBB git hash: 53c1397
  • Database type: mongo
  • Database version:
  • Exact steps to cause this issue:
    use persona with e.g. the lumen theme
    upgrade from 1.9.x
    look at your theme
  • What you expected:
    to look the same as before
  • What happened instead:
    looks different/broken

another big issue for wanting to edit bootswatch themes the body classes no longer includes the actual active skin its just skin-, it should always have the skin name there like before even when the user is using the default skin (default being whatever I selected in the admin panel)

the easiest way to spot this is looking at the headcrumb in the category overview
before https://i.imgur.com/o9qLjRk.png
after https://i.imgur.com/RMYTPvd.png

for now i disabled users switching skin and added the old theme to the custom header

<link id="bootswatchCSS" href="https://maxcdn.bootstrapcdn.com/bootswatch/3.3.7/lumen/bootstrap.min.css" rel="stylesheet" integrity="sha384-gv0oNvwnqzF6ULI9TVsSmnULNb3zasNysvWwfT/s4l8k5I+g6oFz9dye0wg3rQ2Q" crossorigin="anonymous">

i also tried npm i bootswatch@3.3.7 as 3.4.0 was installed by the ^3 set in the package.json but no luck

@barisusakli
Copy link
Member

Seems like the body class never gets added the skin-<skinName> when you change the skin from the ACP.

@julianlam
Copy link
Member

Was positive I didn't remove that logic, but I will check, thanks

julianlam added a commit to NodeBB/nodebb-theme-vanilla that referenced this issue Jan 19, 2019
julianlam added a commit to NodeBB/nodebb-theme-persona that referenced this issue Jan 19, 2019
julianlam added a commit that referenced this issue Jan 19, 2019
@phit
Copy link
Contributor Author

phit commented Jan 19, 2019

alright cherry picking these, seems to work! unfortunately there still seem to be compilation issues, lets take the case of .breadcrumb

img

the padding and background color are all messed up for it and there's tons of issues like this, neither the background nor the padding should be 0

here is the compiled bootswatch css https://github.com/thomaspark/bootswatch/blob/v3.3.7/lumen/bootstrap.css#L4697

actually i looked everywhere in all the less files that should be pulled in and nowhere i could find the reference to that 0

@julianlam
Copy link
Member

Breadcrumbs are wonky on lumen only?

@phit
Copy link
Contributor Author

phit commented Jan 19, 2019

I do not know how other themes looked before, so I have a hard time spotting changes, another thing I've noticed are the font color being #999 in a lot of places where it was a darker black before.

e.g. sandstone
before before image
after after image

you can see in that screenshot even code highlighting broke and the font is just #999 that applies to a lot of themes in various places

youhosi pushed a commit to youhosi/nodebb-theme-oxide that referenced this issue Jan 20, 2019
@phit
Copy link
Contributor Author

phit commented Jan 21, 2019

@julianlam mind reopening so this doesn't get forgotten?

@julianlam
Copy link
Member

Padding is defined zero here: https://github.com/NodeBB/nodebb-theme-persona/blob/master/less/header.less#L271

It seems most of the issues described here are due to overrides in Persona that might no longer be needed now that skins are handled server-side.

@phit
Copy link
Contributor Author

phit commented Jan 28, 2019

okay, so we just have to live will all themes being mostly broken?
heck, even code highlight is not working, unless persona is overriding all code highlight now there's clearly an issue with the compilation

@pitaj
Copy link
Contributor

pitaj commented Jan 28, 2019

The code highlighting issue seems to only apply on a hard load, not when ajaxify is involved. Just tested it on sandstone on the same topic.

@pitaj
Copy link
Contributor

pitaj commented Jan 28, 2019

@phit do you mean all skins?

@pitaj
Copy link
Contributor

pitaj commented Jan 28, 2019

Highlighting is broken even on Default, aka no skin, so something else is going on there.

@phit
Copy link
Contributor Author

phit commented Jan 28, 2019

alright that's hard load then, anyway, yes i meant skins, some kind of order must have changed, as previously bootstrap would override the persona css

@pitaj
Copy link
Contributor

pitaj commented Jan 28, 2019

Could you give some specific examples besides the code highlighting?

@phit
Copy link
Contributor Author

phit commented Jan 28, 2019

padding in general seems to vary wildly from the previous versions, pretty much everywhere

i mean just look at the shift in #7266 (comment)

then theres random font color changes, another easy one to spot is, a:hover had text-decoration: underline; before the update with lumen now it is text-decoration: none;

breadcrumbs as mentioned before is the easiest to spot even if you haven't looked at the skins in detail before

@pitaj
Copy link
Contributor

pitaj commented Jan 28, 2019

The thing is, these changes might actually be because now the skins are actually being applied correctly. Skins are now compiled inside less with the persona theme, so their variable changes and whatnot are more consistently applied.

I think it's likely that what used to be base bootstrap variables in the precompiled skins overriding persona variables are no longer showing up.

@phit
Copy link
Contributor Author

phit commented Jan 28, 2019

sounds about right, but the result is persona overwriting the bootstrap theme and it looks terrible

the fix of applying the skin manually like I do right now shouldn't do anything, right now i put the bootstrap css after the generated stylesheet so it is overwritten and it solves all the issues I'm having..
and it makes the skin look like all other sites where i use bootstrap, persona should not change the look of a bootstrap skin so majorly

@julianlam
Copy link
Member

@pitaj is right, it is because we are now properly loading skins and Persona is overwriting them. However that doesn't indicate a problem with the server-side skin building logic, which is why the issue was closed.

I'd recommend tracking these changes in a new issue so they can be individually looked at and fixed properly.

tdawgtimmy pushed a commit to EnContext/nodebb-theme-encontext that referenced this issue Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants