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

Move all inline style to dedicated endpoint #2877

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vmcj
Copy link
Member

@vmcj vmcj commented Dec 8, 2024

We rendered the <style> within the which is not valid. By moving it to a dedicated endpoint we also save a bit of bandwith as this is content which stays static during the contest.

We rendered the <style> within the <body> which is not valid. By moving
it to a dedicated endpoint we also save a bit of bandwith as this is
content which stays static during the contest.
#[Route(path: '/dynamic-css', name: 'get_dynamic_css')]
public function dynamicCSS(): Response {
$response = new Response();
$response->headers->set('Content-Type', 'text/css');
Copy link
Member

Choose a reason for hiding this comment

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

We should maybe set some expire headers? Otherwise browsers won't cache it.
Drawback is that it will change once the colors change, so maybe use an etag header instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

We discussed and I'll try to implement a cache and keep the etag there.

@@ -100,6 +100,14 @@ public function scoreboardDataZipAction(
return $this->dj->getScoreboardZip($request, $requestStack, $contest, $this->scoreboardService);
}

#[Route(path: '/dynamic-css', name: 'get_dynamic_css')]
Copy link
Member

Choose a reason for hiding this comment

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

The name is too broad. It should be something like scoreboardCategoryColorCss or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

And rename if there are more?

Copy link
Member

Choose a reason for hiding this comment

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

More dynamic CSS? I would put them in separate actions, since otherwise you get one big if here, which doesn't make sense IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change it for now, not sure if we have more pages like this so if the discussion will actually ever be non-theoretical.

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