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

Upgrade main stregsystem pages to modern HTML #481

Merged
merged 7 commits into from
Oct 2, 2024

Conversation

atjn
Copy link
Contributor

@atjn atjn commented Aug 28, 2024

I have removed the old table-based layout and used some CSS instead. I have created two CSS classes that make the layout work much easier:

Main

All pages are now wrapped in a <main> element with default styling which ensures that there is consistent spacing around the edges of each page. Before this, each page implemented different HTML hacks that resulted in different spacing on the pages.

The elements also use the class center which automatically centers most elements on the page. Before this, every single element was wrapped in a <center> element to achieve the same thing.

Horizontal table

Many of the pages used a table to place elements in a horizontal line. I have replaced the tables with a standard class horizontal-table that can apply horizontal layout to any element.

Because I have standardized these styles, the new pages do not look exactly like the old ones did. I don't think the changes are big enough for anyone to really notice, and on the positive side, it is now much easier to apply new styles and theming because everything can be changed with a few CSS classes.

I am sorry to dump such a large PR, but I don't think these changes make sense in isolation, you need to see the full change. If you think it is too big to review, I can split it into smaller changesets and submit those as PR's. Remember to disable whitespace diffs when reviewing, otherwise the changes will seem much bigger than they are.

This is a partial implementation of #473 and #474.

@krestenlaust
Copy link
Member

Have you tested existing themes to make sure they still work?

@atjn
Copy link
Contributor Author

atjn commented Aug 28, 2024

Do you mean the seasonal themes that automatically display on certain dates?

@krestenlaust
Copy link
Member

Yes😁

@atjn
Copy link
Contributor Author

atjn commented Aug 29, 2024

I just checked and they all seem to work fine. They only change the background/foreground, they don't actually interact with the contents of the page, so they are not affected :)

@krestenlaust
Copy link
Member

Awesome, I'll review it when I get the time

Copy link

codecov bot commented Sep 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.90%. Comparing base (6471aeb) to head (29fd2f0).
Report is 14 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next     #481      +/-   ##
==========================================
- Coverage   81.18%   80.90%   -0.28%     
==========================================
  Files          33       39       +6     
  Lines        3114     3609     +495     
  Branches      379      453      +74     
==========================================
+ Hits         2528     2920     +392     
- Misses        546      635      +89     
- Partials       40       54      +14     
Flag Coverage Δ
80.90% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@krestenlaust krestenlaust left a comment

Choose a reason for hiding this comment

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

LGTM!

New appearance:
billede

Old appearance:
billede

@krestenlaust krestenlaust merged commit 0bab04b into f-klubben:next Oct 2, 2024
3 checks passed
@atjn atjn deleted the main-css branch October 2, 2024 10:28
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