💄 [#1169] Make images/icons have static sizes in cards #526

jiromaykin · 2023-03-10T16:00:53Z

issue here: https://taiga.maykinmedia.nl/project/open-inwoner/task/1169
This would solve the problem where 'icons' no longer look like this (with potentially cut-off heads): when the screensize gets smaller than the container - the only way to do this would be to set static sizes for the cards on all screenwidths, and inform the user which sizes need to be used for icons.

Note: this is not for images in Product cards.

ToDo:

set statics max-sizes for Grid columns and Cards: max. of 230 px/columnsize wide, image max-height: 100px. Except for the cards on Registration page (and elsewhere?)

other todo's to make the card align with design will be done later when decision is made about horizontal scroll and about the number of carsds in 1 column on mobile

codecov-commenter · 2023-03-13T10:52:43Z

Codecov Report

Merging #526 (e0bf5ac) into develop (7498774) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #526   +/-   ##
========================================
  Coverage    96.47%   96.47%           
========================================
  Files          539      540    +1     
  Lines        19211    19216    +5     
========================================
+ Hits         18534    18539    +5     
  Misses         677      677

Impacted Files	Coverage Δ
src/open_inwoner/pdc/views.py	`88.10% <100.00%> (ø)`

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

alextreme · 2023-03-17T08:54:30Z

@jiromaykin laten we deze zo samen even doornemen

jiromaykin · 2023-03-17T11:17:35Z

@alextreme We could decide to add this version for now, as a form on 'intermediate but improved' state - this version doesn't have the 'fallback' option removed yet though - should we make it required to always choose an icon?

alextreme · 2023-03-17T11:36:20Z

@alextreme We could decide to add this version for now, as a form on 'intermediate but improved' state - this version doesn't have the 'fallback' option removed yet though - should we make it required to always choose an icon?

No, as discussed on monday we can keep the fallback option as-is, as long as we don't cause weird aspect ratio's anymore this should be fine.

Happy to merge if you and Vasileios are happy with this

jiromaykin · 2023-03-17T11:57:50Z

This intermediate stage still needs to be reviewed.

vaszig

I am a bit confused with this PR. I understood that we want the changes only for the theme-cards and not for the product ones (which will be fixed in another PR). If I am wrong let me know please.

vaszig · 2023-03-17T15:32:57Z

src/open_inwoner/pdc/views.py

@@ -60,7 +60,7 @@ def page_title(self):
    def get_context_data(self, **kwargs):
        config = SiteConfiguration.get_solo()

-        limit = 3 if self.request.user.is_authenticated else 4
+        limit = 4


We can remove this variable now as the limit is not dynamic any more. Let's use the number directly in each queryset.

In future I may have to make a difference between the mobile limit and the desktop limit, which hopefully can be done in the templates somewhere (not sure yet), so I guess this can be cleaned up.

@vaszig I now see the 'limit' is used 5 times in this file; is it really a good idea to remove the 'limit' variable? I would think it is cleaner to keep it and reuse it instead of the value? (in javascript I would keep the variable, not sure if this is also Python convention).

I would use a variable if I wanted first to evaluate something and then reuse it (Not in Python, in every language). Here we don;t need to evaluate something, it's just an integer. But this is my knowledge, @Bartvaderkin can you tell us your opinion please?

@vaszig @jiromaykin I would keep the variable, so we can see all the usages are linked to the same value. If we'd put the plain number everywhere you get a magic number situation where it isn't clear which numbers are related or just happen to have the same value.

@Bartvaderkin @vaszig Hoorrah! in Javascript I would use a 'const' for numbers like these - so I declare this PR as finished and ready for review again :-)

jiromaykin · 2023-03-17T15:47:47Z

I am a bit confused with this PR. I understood that we want the changes only for the theme-cards and not for the product ones

This is a PR that will change overall responsiveness of all of the columns so everything looks better already before continuing to fine-tuning later; so this one is more about the weird resizing of ALL of the card-columns -this means the Theme card images will remain stable - the Product card images don't need responsiveness for now so they don't need to change here;
Example: if you change screenwidths gradually in the old version it's all very 'elastic' but in this PR all cards have become 'block like' and will drop to the next line if the screenwidth becomes smaller - hope this helps a bit.

old columns that need to become 'static' on mobile example:

jiromaykin · 2023-03-23T12:55:01Z

@vaszig This may not look very impressive, but hopefully this works well as an in-between stage with some new CSS classes for future views; hopefully I haven't made any columns/views act all weird here.

vaszig · 2023-03-23T14:44:41Z

src/open_inwoner/scss/views/Home.scss

+
+.home {
+  .card-container {
+    grid-template-columns: repeat(auto-fit, 228px);


Maybe we could add a specific rule for the mobile version as it seems a bit weird to my eyes, but you know better.

@vaszig For mobile: horizontal scrolling will be added later in a different issue so it will look better, but the cards will need to remain this same size.

What do you think @alextreme ?

This is fine for now, the scrolling is something Jiro will continue on but these changes are a necessary first step for that.

jiromaykin added the discussion Marked for discussion label Mar 13, 2023

jiromaykin force-pushed the feature/1169-make-images-have-static-size-in-cards branch from 18985b0 to 5232c1b Compare March 17, 2023 10:37

jiromaykin removed the discussion Marked for discussion label Mar 17, 2023

jiromaykin marked this pull request as ready for review March 17, 2023 12:52

alextreme requested a review from vaszig March 17, 2023 13:12

vaszig requested changes Mar 17, 2023

View reviewed changes

jiromaykin requested review from Bartvaderkin and vaszig March 20, 2023 12:05

Bartvaderkin approved these changes Mar 20, 2023

View reviewed changes

jiromaykin marked this pull request as draft March 20, 2023 14:08

jiromaykin force-pushed the feature/1169-make-images-have-static-size-in-cards branch from f85ec93 to 7aa99f7 Compare March 20, 2023 14:17

💄 [#1169] Make images/icons have static sizes in cards

111a455

jiromaykin force-pushed the feature/1169-make-images-have-static-size-in-cards branch from eb4f9ee to 111a455 Compare March 23, 2023 11:22

jiromaykin marked this pull request as ready for review March 23, 2023 11:58

vaszig approved these changes Mar 23, 2023

View reviewed changes

alextreme merged commit 9d9d0c3 into develop Mar 23, 2023

alextreme deleted the feature/1169-make-images-have-static-size-in-cards branch March 23, 2023 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

💄 [#1169] Make images/icons have static sizes in cards #526

💄 [#1169] Make images/icons have static sizes in cards #526

jiromaykin commented Mar 10, 2023 •

edited

Loading

codecov-commenter commented Mar 13, 2023 •

edited

Loading

alextreme commented Mar 17, 2023

jiromaykin commented Mar 17, 2023

alextreme commented Mar 17, 2023

jiromaykin commented Mar 17, 2023

vaszig left a comment

vaszig Mar 17, 2023

jiromaykin Mar 17, 2023

jiromaykin Mar 20, 2023

vaszig Mar 20, 2023

Bartvaderkin Mar 20, 2023

jiromaykin Mar 20, 2023

jiromaykin commented Mar 17, 2023

jiromaykin commented Mar 23, 2023

vaszig Mar 23, 2023

jiromaykin Mar 23, 2023

jiromaykin Mar 23, 2023

alextreme Mar 23, 2023

💄 [#1169] Make images/icons have static sizes in cards #526

💄 [#1169] Make images/icons have static sizes in cards #526

Conversation

jiromaykin commented Mar 10, 2023 • edited Loading

codecov-commenter commented Mar 13, 2023 • edited Loading

Codecov Report

alextreme commented Mar 17, 2023

jiromaykin commented Mar 17, 2023

alextreme commented Mar 17, 2023

jiromaykin commented Mar 17, 2023

vaszig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiromaykin commented Mar 17, 2023

old columns that need to become 'static' on mobile example:

jiromaykin commented Mar 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiromaykin commented Mar 10, 2023 •

edited

Loading

codecov-commenter commented Mar 13, 2023 •

edited

Loading