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

[FIX] Loading theme CSS on first server startup #13953

Merged
merged 1 commit into from
Apr 2, 2019

Conversation

sampaiodiego
Copy link
Member

Change the technique used to serve the theme.css file.

Previous code was overwriting an internal meteor function that read/validate all assets of the app and if perhaps the CSS theme was already "ready" (the less was compiled), it was then added to assets list. This was causing an issue though, that on the first start of a server, where the compiled CSS was not yet saved on DB, then the theme.css was being served. Causing the UI to look like this:

image

Previously, to fix the above issue a restart was required.

@sampaiodiego sampaiodiego added this to the 1.0.0 milestone Mar 29, 2019
@sampaiodiego sampaiodiego changed the title [IMPROVE] Inject tag to load theme.css [FIX] Loading theme CSS on first load Apr 1, 2019
@sampaiodiego sampaiodiego changed the title [FIX] Loading theme CSS on first load [FIX] Loading theme CSS on first server startup Apr 1, 2019
res.setHeader('Content-Type', 'text/css; charset=UTF-8');
res.setHeader('Content-Length', currentSize);
res.setHeader('ETag', `"${ currentHash }"`);
res.write(theme.getCss());
Copy link
Member

Choose a reason for hiding this comment

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

Would this actually exist?

I know let is defined outside this scope... but will this actually persist?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes =)

Variables declared by let have their scope in the block for which they are defined, as well as in any contained sub-blocks.

source

Copy link
Member Author

Choose a reason for hiding this comment

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

actually, if you're worried about theme variable, it is a const that was being used before as well

@sampaiodiego sampaiodiego merged commit 42b57f9 into develop Apr 2, 2019
@sampaiodiego sampaiodiego deleted the fix-first-theme-css-load branch April 2, 2019 00:52
@facundomedica
Copy link

@sampaiodiego could this be the reason that breaks the loading of theme.css when using Rocketchat in a subdirectory? I'm testing on develop branch and found out that now it asks for /theme.css which doesn't exist.

@sampaiodiego
Copy link
Member Author

@sampaiodiego could this be the reason that breaks the loading of theme.css when using Rocketchat in a subdirectory? I'm testing on develop branch and found out that now it asks for /theme.css which doesn't exist.

I can take a look..

@facundomedica
Copy link

facundomedica commented Apr 5, 2019

Thanks for the quick response! More info: It also happens in 1.0.0-rc.0 but not in 1.0.0-beta.2

@sampaiodiego
Copy link
Member Author

@facundomedica can you please help testing #14015 ?

@facundomedica
Copy link

@sampaiodiego thank you, is there a way to have this uploaded to docker hub? I've been testing from there and I don't have the set up to run it in other way

@sampaiodiego
Copy link
Member Author

@facundomedica you can test with docker image rocketchat/rocket.chat:pr-14015 .. from my tests it is now ok, so went ahead and merged the PR.

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.

None yet

5 participants