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

The link on Alert is hard to read #11446

Closed
deniapps opened this issue Apr 7, 2023 · 13 comments
Closed

The link on Alert is hard to read #11446

deniapps opened this issue Apr 7, 2023 · 13 comments
Assignees
Labels
Milestone

Comments

@deniapps
Copy link

deniapps commented Apr 7, 2023

image

As per the documentation, "Use the .alert-link utility class to quickly provide matching colored links within any alert."

It appears to be a simple fix by adding the "alert-link" class.

@julianlam
Copy link
Member

Thanks @deniapps! That solution looks to be fairly straightforward 😄

@barisusakli barisusakli self-assigned this Apr 7, 2023
@barisusakli barisusakli added the bug label Apr 7, 2023
@barisusakli barisusakli added this to the 3.0.0 milestone Apr 7, 2023
@barisusakli
Copy link
Member

barisusakli commented Apr 7, 2023

@deniapps which skin was the one in the screenshot?

@deniapps
Copy link
Author

deniapps commented Apr 7, 2023

@deniapps which skin was the one in the screenshot?

I use "Yeti", but it seemed to be everywhere. It appears to have been resolved now. That was quick!!!

By the way, switching skins is quite slow. I understand that it may be challenging to make it faster. But I just noticed that the paintbrush icon is flashing when changing skins. :-) Perhaps using a more noticeable loading icon would be helpful.

@julianlam
Copy link
Member

Yes, unfortunately that is the byproduct of NodeBB needing to recompile the CSS with the new SCSS variables when a new skin is selected.

Once compiled they are cached for future skin changes.

We don't recompile the stylesheet for all skins, mostly to reduce the build time.

@barisusakli
Copy link
Member

We could compile all skins on startup asynchronously so it doesn't make build longer but if you are running on a single core it would make the forum a bit slower after restart.

@julianlam
Copy link
Member

Yeah I think it'd be a tough ask given that compiling the SCSS is something that would (I imagine) take up the entirety of the CPU's time... And multiply that by how many skins Bootswatch supports 🤔

@deniapps
Copy link
Author

deniapps commented Apr 8, 2023

I don't see any hash class names in the style files for the skins, such as https://community.nodebb.org/assets/client-cosmo.css or https://community.nodebb.org/assets/client.css.

It seems that these files may have been pre-compiled and bundled with the theme. If any other styles need to be regenerated during the build process, a separate CSS file may be utilized.

I may have missed something though.

@barisusakli
Copy link
Member

Each skin is its own separate css file, compiled by adding bootstrap and our own styles together. WHen a new skin is requested it was generated on demand which can take some time on the first build. I ve added a change into harmony that builds all the skins on startup in production mode NodeBB/nodebb-theme-harmony@6a8aa39.

It takes about 3 minutes to build all 25 skins, but it doesn't block startup and happens on a separate thread so I think it's fine.

image

@deniapps
Copy link
Author

@barisusakli The async build sounds good, but does it mean that every time we update some custom CSS, it will require us to rebuild all those files? It seems to me that we don't edit those skin CSS files directly, then why not have them as a separated <style> in the main template, for example: <link rel="stylesheet" href="/asset/skins/cosmo/bootstrap.css">? The changing skin only needs to change the link to the corresponding skin file, and does not need to rebuild everything. Am I missing something?

@barisusakli
Copy link
Member

Rules in the custom css tab aren't used in the build process they are applied after all other rules, so changing css rules in the ACP won't rebuild everything.

That's how it works right now, when you change the skin from the dropdown in harmony it updates the rel="stylesheet" to the new css file.

@deniapps
Copy link
Author

Okay. Thank you for the explanation.

@deniapps
Copy link
Author

@barisusakli how to apply this to rc2? Should I pull the develop branch, then do the upgrade? or manually update nodebb-theme-harmony in the package.json?

@barisusakli
Copy link
Member

On develop branch git pull, then do a ./nodebb upgrade it will install the harmony version from install/package.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants