[#1798] Refactored document list in case detail view #825

pi-sigma · 2023-10-25T13:30:24Z

added creation date to documents
added indicator for recently added documents ("recently added" made configurable)

TODO:

the size of the document icon on the left needs to adjust dynamically, depending on whether the "recently added" indicator is present or not (currently the size is hardwired via padding)

Bartvaderkin · 2023-10-31T08:22:25Z

Please fix the commit message and PR title. "Feature/1798 document list" doesn't mean a lot.

jiromaykin

Requesting quite a few changes 🙂 - if some of it is too difficult, I can help, but this week I'm very busy with other things.

src/open_inwoner/scss/components/File/File.scss

src/open_inwoner/components/templates/components/File/File.html

src/open_inwoner/templates/pages/cases/status_inner.html

jiromaykin

Preferably do not use BR for styling, it has no structural meaning and cannot easily be overwritten by CSS.
In this case it causes a problem: we cannot change some element's padding/margin, so this does not conform to the design.

The structure that can use Flexbox should look a bit more more like this (see attachment), without any BR's, but make sure it still looks good on mobile + some columns need to be set to align-items: center; when needed, like: depending on wether they actually have a 'recently_added' element or not.

src/open_inwoner/components/templates/components/File/File.html

jiromaykin · 2023-11-02T10:54:19Z

src/open_inwoner/scss/components/File/File.scss

+  &__symbol {
+    display: flex;
+    align-items: center;
+    background-color: var(--color-gray-lightest);
    padding: var(--spacing-large);


This old padding now needs to change because the icon also adds side-padding:
padding: var(--spacing-large) 0;

Please change the old padding by setting the side-padding to zero/0 🙂 and like i said the the earlier comment

This is still not correct.
Please add the zero to remove unnecessary padding to the left/right sides here.
(padding shorthand notation here = top/bottom right/left)

See the differences:

src/open_inwoner/components/templates/components/File/File.html

jiromaykin

I know flexbox wrapping is hard to 'wrap' your mind around; just a few more changes and this is done!

src/open_inwoner/components/templates/components/File/File.html

jiromaykin · 2023-11-06T11:19:46Z

src/open_inwoner/scss/components/File/File.scss

+  &__symbol {
+    display: flex;
+    align-items: center;
+    background-color: var(--color-gray-lightest);
    padding: var(--spacing-large);


Please change the old padding by setting the side-padding to zero/0 🙂 and like i said the the earlier comment

src/open_inwoner/scss/components/File/File.scss

codecov-commenter · 2023-11-10T08:46:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5cb8198) 92.88% compared to head (b199501) 92.89%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #825   +/-   ##
========================================
  Coverage    92.88%   92.89%           
========================================
  Files          767      768    +1     
  Lines        26445    26478   +33     
========================================
+ Hits         24563    24596   +33     
  Misses        1882     1882

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

jiromaykin

I understand the flex-box thing is tricky, but please do not use BR's.

src/open_inwoner/components/templates/components/File/File.html

jiromaykin · 2023-11-20T11:31:01Z

src/open_inwoner/scss/components/File/File.scss

+  &__symbol {
+    display: flex;
+    align-items: center;
+    background-color: var(--color-gray-lightest);
    padding: var(--spacing-large);


This is still not correct.
Please add the zero to remove unnecessary padding to the left/right sides here.
(padding shorthand notation here = top/bottom right/left)

See the differences:

- Added date - Added 'new' indicator to documents created no more than 24h in the past

pi-sigma · 2023-11-21T08:32:20Z

@jiromaykin Styling is fixed, thanks for the input. The design doesn't have the text ("Download") in the download link, but I was wondering if it's better to keep it for consistency and accessibility. What do you think?

jiromaykin · 2023-11-21T09:02:19Z

"....The design doesn't have the text ("Download") in the download link, but I was wondering if it's better to keep it for consistency and accessibility. What do you think?...."

For accessibility this is not quite an issue: any text can be made invisible while remaining in the structure (with CSS you can use transform/opacity/positioning to make elements visible for screenreader-software only). Also: aria-label and title attribute can be used for tooltips.

svenvandescheur · 2023-11-21T13:31:48Z

src/open_inwoner/cms/cases/views/status.py

@@ -201,6 +206,11 @@ def get_context_data(self, **kwargs):
                "end_statustype_data": end_statustype_data,
                "documents": documents,
                "allowed_file_extensions": sorted(config.allowed_file_extensions),
+                "new_docs": has_new_elements(


Cant we solve this dynamically within the FileList component?

@alextreme @pi-sigma as discussed: the template tag implementation might not be the most favorable approach and we might want to move away from that pattern. This does raises some issues in terms of reusability and complexity which might needs to be discusses in a separate team-wide meeting.

Some concerns are:

How can we guarantee consistency of UI elements across different pages.

Previously the template tag would take a predefined "(API) interface" and would render a specialized, reusable template for that component. This allowed Django templates to be built much like other modern component based frontend tooling. The template tag approach was a more developed leftover from porting a pure frontend (Vite?) implementation but felt confusing to developers less familiar with such concepts.

Currently, some pages move away from this strategy and use a more classic Django view approach. As a result, certain (HTML) code paths might act different on different pages causing potential UI inconsistencies and DRY violations.

How can we keep context processing maintainable.

In order to facilitate all the individual frontend components, quite a lot of context processing is necessary. When moving away from template tags, this typically ends up in the get_context_data function in the views.

Looking at

open-inwoner/src/open_inwoner/cms/cases/views/status.py

Line 117 in 38ced6b

def get_context_data(self, **kwargs):

I have some concerns about the complexity and maintainability of this processing, I't very unclear to me:

What certain processing is for.

If and where certain context is used.

What the expected shape of data is within templates

How can we keep developers happy about this setup.

All approaches have advantages and drawback but we need to find a solution that works for all team members.

pi-sigma force-pushed the feature/1798-document-list branch 6 times, most recently from f85af6a to 0a99819 Compare October 30, 2023 11:16

pi-sigma marked this pull request as ready for review October 30, 2023 13:11

pi-sigma force-pushed the feature/1798-document-list branch from 0a99819 to 9ace98a Compare October 30, 2023 13:37

pi-sigma changed the title ~~Feature/1798 document list~~ [#1798] Refactored document list in case detail view Oct 31, 2023

pi-sigma requested a review from jiromaykin October 31, 2023 10:30

jiromaykin requested changes Oct 31, 2023

View reviewed changes

pi-sigma force-pushed the feature/1798-document-list branch from 9ace98a to b64f3e2 Compare November 1, 2023 10:30

pi-sigma requested a review from jiromaykin November 1, 2023 15:38

jiromaykin requested changes Nov 2, 2023

View reviewed changes

alextreme requested a review from jiromaykin November 6, 2023 09:35

pi-sigma force-pushed the feature/1798-document-list branch from b64f3e2 to ec141e2 Compare November 6, 2023 10:38

jiromaykin requested changes Nov 6, 2023

View reviewed changes

pi-sigma force-pushed the feature/1798-document-list branch 4 times, most recently from 0b49b8a to 16b7d66 Compare November 10, 2023 08:38

pi-sigma requested a review from jiromaykin November 10, 2023 12:58

jiromaykin requested changes Nov 20, 2023

View reviewed changes

[#1798] Updated document list element

b199501

- Added date - Added 'new' indicator to documents created no more than 24h in the past

pi-sigma force-pushed the feature/1798-document-list branch from 16b7d66 to b199501 Compare November 21, 2023 08:21

jiromaykin requested a review from svenvandescheur November 21, 2023 12:47

svenvandescheur added 3 commits November 21, 2023 14:24

💄 #1798 - fix: fix text overflow on labels.

facf87f

💄 #1798 - feat: use outlined icon for download button.

528ca4a

💄 #1798 - feat: make the file icon correctly scale vertically.

38ced6b

svenvandescheur requested changes Nov 21, 2023

View reviewed changes

svenvandescheur approved these changes Nov 21, 2023

View reviewed changes

jiromaykin self-requested a review November 21, 2023 14:08

jiromaykin approved these changes Nov 21, 2023

View reviewed changes

svenvandescheur merged commit df90e8c into develop Nov 21, 2023
14 checks passed

svenvandescheur deleted the feature/1798-document-list branch November 21, 2023 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1798] Refactored document list in case detail view #825

[#1798] Refactored document list in case detail view #825

pi-sigma commented Oct 25, 2023 •

edited

Loading

Bartvaderkin commented Oct 31, 2023

jiromaykin left a comment

jiromaykin left a comment

jiromaykin Nov 2, 2023

jiromaykin Nov 6, 2023

jiromaykin Nov 20, 2023

jiromaykin left a comment

jiromaykin Nov 6, 2023

codecov-commenter commented Nov 10, 2023 •

edited

Loading

jiromaykin left a comment

jiromaykin Nov 20, 2023

pi-sigma commented Nov 21, 2023

jiromaykin commented Nov 21, 2023

svenvandescheur Nov 21, 2023

svenvandescheur Nov 21, 2023

[#1798] Refactored document list in case detail view #825

[#1798] Refactored document list in case detail view #825

Conversation

pi-sigma commented Oct 25, 2023 • edited Loading

Bartvaderkin commented Oct 31, 2023

jiromaykin left a comment

Choose a reason for hiding this comment

jiromaykin left a comment

Choose a reason for hiding this comment

jiromaykin Nov 2, 2023

Choose a reason for hiding this comment

jiromaykin Nov 6, 2023

Choose a reason for hiding this comment

jiromaykin Nov 20, 2023

Choose a reason for hiding this comment

jiromaykin left a comment

Choose a reason for hiding this comment

jiromaykin Nov 6, 2023

Choose a reason for hiding this comment

codecov-commenter commented Nov 10, 2023 • edited Loading

Codecov Report

jiromaykin left a comment

Choose a reason for hiding this comment

jiromaykin Nov 20, 2023

Choose a reason for hiding this comment

pi-sigma commented Nov 21, 2023

jiromaykin commented Nov 21, 2023

svenvandescheur Nov 21, 2023

Choose a reason for hiding this comment

svenvandescheur Nov 21, 2023

Choose a reason for hiding this comment

pi-sigma commented Oct 25, 2023 •

edited

Loading

codecov-commenter commented Nov 10, 2023 •

edited

Loading