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

[#2042] Fix margins for Userfeed and Home #1048

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

jiromaykin
Copy link
Contributor

@jiromaykin jiromaykin commented Feb 22, 2024

issue: https://taiga.maykinmedia.nl/project/open-inwoner/task/2042

This is a first start to begin transforming the main grid in such a way that 'components no longer have outside margins around themselves - the parent grid should determine the spacing between components, not the components themselves - see also the temporary margin-top fix here that was the original reason for this PR.

This PR also enables spacing between plugins on the Home page by wrapping them with a Section - so you can move around any of the plug-ins in any order without having to constantly update any combination of margins between cards & heading, between cards and questionnaires, between userfeed and video, etc, etc.

@jiromaykin jiromaykin force-pushed the fix/2042-fix-home-userfeed-margins branch from 6dea4fb to 654faa0 Compare February 22, 2024 17:14
@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.91%. Comparing base (5e6f11e) to head (c6dda1a).

❗ Current head c6dda1a differs from pull request most recent head 02de82f. Consider uploading reports for the commit 02de82f to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1048   +/-   ##
========================================
  Coverage    94.91%   94.91%           
========================================
  Files          882      882           
  Lines        30792    30792           
========================================
  Hits         29227    29227           
  Misses        1565     1565           

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

@jiromaykin jiromaykin force-pushed the fix/2042-fix-home-userfeed-margins branch 2 times, most recently from 2292a68 to c6dda1a Compare February 22, 2024 19:34
@jiromaykin jiromaykin force-pushed the fix/2042-fix-home-userfeed-margins branch from 3878464 to 02de82f Compare February 22, 2024 19:56
@jiromaykin jiromaykin marked this pull request as ready for review February 22, 2024 21:11
Copy link
Contributor

@pi-sigma pi-sigma left a comment

Choose a reason for hiding this comment

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

Looking good. I do wonder about the styling convention. h1 elements are now styled as utrecht-heading-1 but h2 elementss continue to receive class="h2"? Is that an oversight or intentional? Just so I know which convention to follow when I write this myself...

@jiromaykin
Copy link
Contributor Author

Looking good. I do wonder about the styling convention. h1 elements are now styled as utrecht-heading-1 but h2 elementss continue to receive class="h2"? Is that an oversight or intentional? Just so I know which convention to follow when I write this myself...

Currently this was intentional - I need another design-token sprint in order to create more values from NL Design System classes that we can use. Also: in Open-Inwoner we have a bit of a complicated H2 element that uses flexbox in order to get 'indicator' icons and sometimes the H2 wraps itself around a button so H2 in particular is more complicated to completely rework; the other Headings will be much more straightforward.

@alextreme alextreme merged commit db9db6f into develop Feb 26, 2024
15 checks passed
@alextreme alextreme deleted the fix/2042-fix-home-userfeed-margins branch February 26, 2024 17:56
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.

5 participants