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

💄 [#1132] Select-element caret for contacts-page #503

Closed

Conversation

jiromaykin
Copy link
Contributor

@jiromaykin jiromaykin commented Feb 27, 2023

select tag needs arrow/caret
https://taiga.maykinmedia.nl/project/open-inwoner/issue/1132

After discussing with Roxanne (designer):

  • translation/textually "soort contact" and "type contact" should be the same; so pick one and use consistently (TODO later in translations), either use "soort contact" everywhere, or use "type contact" everywhere.
  • align the Select element to the right
  • add icon with expand_more arrow/caret
  • discuss with designer if all filter-pages should look consistent
  • delete the old PR/branch due to mess with translations and wrong name there
  • refactor Filters section to look like all the others (like in Plans and Actions)

@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2023

Codecov Report

Merging #503 (3e9c0f7) into develop (4b09a8c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #503   +/-   ##
========================================
  Coverage    96.48%   96.48%           
========================================
  Files          527      527           
  Lines        19003    19003           
========================================
  Hits         18335    18335           
  Misses         668      668           
Impacted Files Coverage Δ
.../open_inwoner/components/templatetags/form_tags.py 94.30% <ø> (ø)

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

@jiromaykin jiromaykin marked this pull request as ready for review February 27, 2023 14:04
@jiromaykin jiromaykin force-pushed the feature/1132-select-element-for-contacts-page branch from 005898c to fddb998 Compare February 28, 2023 09:31
@jiromaykin jiromaykin changed the title 🎨 [#1132] select-element caret for contacts-page 🎨 [#1132] Select-element caret for contacts-page Feb 28, 2023
@jiromaykin jiromaykin changed the title 🎨 [#1132] Select-element caret for contacts-page 💄 [#1132] Select-element caret for contacts-page Feb 28, 2023
@jiromaykin jiromaykin requested a review from vaszig February 28, 2023 09:38
Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

It looks nice on desktop version but what about the mobile one? (there is no space between the two elements and is the select correctly aligned?)

image

@jiromaykin
Copy link
Contributor Author

It looks nice on desktop version but what about the mobile one? (there is no space between the two elements and is the select correctly aligned?)

Thank you - for now I have changed this one to look more like the other pages that have a filter - but this means we are repeating the same pattern for Plans/Actions/Contacts - so in future I probably should refactor them into 1 single JS file (not sure yet...).

@jiromaykin jiromaykin requested a review from vaszig March 2, 2023 15:22
<div class="contacts">
{% render_form id="contact-filter" form=form method="GET" no_actions=True horizontal=True spaceless=True autosubmit=True form_action=form_action class="form form--columns-2 form--spaceless form--autosubmit form--horizontal" %}
<div class="form__control contacts__filters">
<div class="contacts__filter-button">
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to keep the names consistent. The parent is different from the child (contacts__filters and contacts__filter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:-D I was using the naming convention from the 'Actions' files open_inwoner/js/components/actions/index.js - where it seems a collection of filters is called as the multiple 'filters', but I can change this, no prob.
I will then also have to change this in the Actions files, or else we will have different conventions.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem then, you can keep it as is.

@vaszig
Copy link
Contributor

vaszig commented Mar 3, 2023

Some of my comments should be present in my previous review but I was lost in the html changes (formatting) and didn't mention them.

@jiromaykin jiromaykin marked this pull request as draft March 3, 2023 09:20
@jiromaykin jiromaykin marked this pull request as ready for review March 3, 2023 14:24
@jiromaykin jiromaykin marked this pull request as draft March 5, 2023 14:44
@jiromaykin jiromaykin marked this pull request as ready for review March 5, 2023 15:36
@jiromaykin
Copy link
Contributor Author

@vaszig @Bartvaderkin I was trying to solve a merge conflict by rebasing so hopefully this all went correct!
This looks a bit crazy now, with all the rebased commits, but my own code should be correct...

@Bartvaderkin
Copy link
Contributor

Bartvaderkin commented Mar 6, 2023

@jiromaykin This rebase didn't really go as it should, your PR shouldn't list all other commits. Also merging from develop back into a feature branch is not what we'd like to see (this really messes with the git history).

If you can't fix this branch it is also an option to make a new branch and do things like Cherry-Pick from this, or if all else fails just manually copy your changes (using Browse At Revision).

@jiromaykin jiromaykin marked this pull request as draft March 6, 2023 10:11
@jiromaykin
Copy link
Contributor Author

...it is also an option to make a new branch and do things like Cherry-Pick from this, or if all else fails just manually copy your changes (using Browse At Revision).

Thanks - this branch has become a mess so I will start a new one - sorry for this!

@jiromaykin jiromaykin closed this Mar 6, 2023
@jiromaykin jiromaykin deleted the feature/1132-select-element-for-contacts-page branch March 7, 2023 09:32
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