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

💄 [#2685] Add snailmail/digital notifications design #1357

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

jiromaykin
Copy link
Contributor

@jiromaykin jiromaykin commented Aug 19, 2024

Multi radio select design (lots of switching between decisions on whether to use icons or not)
taiga: https://taiga.maykinmedia.nl/project/open-inwoner/task/2685

So needs to look like this, (but with the checkmark for Acties on the right instead of left):

Screenshot 2024-08-26 at 15 26 48

Using the single radio Parent-select component just like these: https://github.com/maykinmedia/open-inwoner/pull/1043/files

Test:
http://localhost:8000/mijn-profiel/notificaties/
http://localhost:8000/mijn-profiel/register/necessary/
http://localhost:8000/kvk/branches/

If we ever need to implement the other vertical design (without the icons) in future, we can use this very same template in different ways, like this:

{% choice_listsingle=False cols=False %}
    {% for choice in field.field.choices %}
        {% choice_list_item input_type_radio=False choice=choice name=field.name data=field.value index=forloop.counter initial=field.form.initial icon_symbol=False %}
    {% endfor %}
{% endchoice_list %}

.....
    {% choice_list single=True  cols=True %}
        {% for choice in field.field.choices %}
            {% choice_list_item input_type_radio=True choice=choice name=field.name data=field.value index=forloop.counter initial=field.form.initial  icon_symbol=choice.1|get_icon_symbol %}
        {% endfor %}
    {% endchoice_list %}

@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.21%. Comparing base (b9e1ac8) to head (286b3ca).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1357   +/-   ##
========================================
  Coverage    95.21%   95.21%           
========================================
  Files         1005     1005           
  Lines        37185    37189    +4     
========================================
+ Hits         35404    35408    +4     
  Misses        1781     1781           

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

@jiromaykin jiromaykin force-pushed the feature/2685-snailmail-notification-design branch 2 times, most recently from fa03aab to fcef131 Compare August 20, 2024 09:20
@jiromaykin jiromaykin changed the title [#2685] Add snailmail/digital notifications design 💄 [#2685] Add snailmail/digital notifications design Aug 20, 2024
@@ -12,7 +12,7 @@ def icon(icon, **kwargs):
Fontawesome Brands: https://fontawesome.com/v5.15/icons?d=gallery&p=2&s=brands

Usage:
{% icon "arrow-forward" %}
{% icon "arrow-forward" outlined=True %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know this is just documentation, but this way it reminds other developers to not forget we use the outlined (not filled) versions.

@jiromaykin jiromaykin force-pushed the feature/2685-snailmail-notification-design branch 4 times, most recently from 2e9c24b to 5926532 Compare August 27, 2024 10:35
@jiromaykin jiromaykin force-pushed the feature/2685-snailmail-notification-design branch from 9ae19e6 to 319ec9b Compare August 27, 2024 14:08
@jiromaykin jiromaykin marked this pull request as ready for review August 27, 2024 14:31
@jiromaykin
Copy link
Contributor Author

Oh yeah: @pi-sigma @swrichards If we don't need the template tag, I could remove it but... this way I think it's clearer we can re-use this + I did not style the vertical checkbox option with icons for the full 100%, because I don't think we will ever use that version, but at least this will make it easier to switch.
Also I did not use this component for the KVK branches page because that one has more fields and is slightly different with the clickable labels.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure the current setting is properly synced upon loading the profile:

Screencast.from.2024-08-27.16-58-14.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are (potentially) several issues:

  • the "Communicatiekanaal" in the profile is at least misleading because it doesn't reflect the user's choice when they sign up or change their profile. However, I'm not sure if this should be addressed or how, since the choice only concerns notifications about zaak notificaties where action is required (there is no post for notifications about actions or messages)
  • it would be nice if the radio was pre-selected according to the user's choice. So if I sign up with "Digitaal en per brief", the corresponding radio choice is pre-selected when I edit my profile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, reality is not represented nicely for the "email" thing (also: not all municipalities will be using this), so I've created a new issue which I can pick up after completing this PR properly (preferring to keep development on other components separately): https://taiga.maykinmedia.nl/project/open-inwoner/issue/2709

src/open_inwoner/components/templatetags/form_tags.py Outdated Show resolved Hide resolved
+ data: the value of a form field field
- index: the index of a for-loop when looping over choices
- initial: the initial value of the field
- input_type_radio: bool | if this is true, show radiobutton, else show checkbox
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's better to use input_type: str and do {% choice_list_item ... input_type=radio %} and {% if input_type == "radio" %} respectively. This way the template and template tag could be extended to work with other input types (like switch). Just an idea, though; perhaps it's not worth the effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are (potentially) several issues:

  • the "Communicatiekanaal" in the profile is at least misleading because it doesn't reflect the user's choice when they sign up or change their profile. However, I'm not sure if this should be addressed or how, since the choice only concerns notifications about zaak notificaties where action is required (there is no post for notifications about actions or messages)
  • it would be nice if the radio was pre-selected according to the user's choice. So if I sign up with "Digitaal en per brief", the corresponding radio choice is pre-selected when I edit my profile.

@jiromaykin jiromaykin force-pushed the feature/2685-snailmail-notification-design branch from e7c2716 to 286b3ca Compare August 29, 2024 10:10
@jiromaykin jiromaykin merged commit c686d9d into develop Sep 5, 2024
18 checks passed
@jiromaykin jiromaykin deleted the feature/2685-snailmail-notification-design branch September 5, 2024 12:03
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.

4 participants