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

[#1160] Add cropping for profile image #523

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

vaszig
Copy link
Contributor

@vaszig vaszig commented Mar 8, 2023

No description provided.

@codecov-commenter
Copy link

Codecov Report

Merging #523 (ac3061b) into develop (4b09a8c) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #523      +/-   ##
===========================================
- Coverage    96.48%   96.47%   -0.01%     
===========================================
  Files          527      528       +1     
  Lines        19003    19040      +37     
===========================================
+ Hits         18335    18369      +34     
- Misses         668      671       +3     
Impacted Files Coverage Δ
src/open_inwoner/accounts/admin.py 94.04% <100.00%> (+0.07%) ⬆️
src/open_inwoner/accounts/forms.py 97.85% <100.00%> (+<0.01%) ⬆️
...ner/accounts/migrations/0056_auto_20230307_0852.py 100.00% <100.00%> (ø)
src/open_inwoner/accounts/models.py 97.87% <100.00%> (+0.01%) ⬆️
.../open_inwoner/components/templatetags/form_tags.py 93.65% <100.00%> (-0.66%) ⬇️
src/open_inwoner/conf/base.py 95.18% <100.00%> (+0.11%) ⬆️
src/open_inwoner/utils/ckeditor.py 75.00% <0.00%> (-17.31%) ⬇️
...rc/open_inwoner/pdc/tests/test_product_location.py 100.00% <0.00%> (ø)
.../open_inwoner/components/templatetags/link_tags.py 100.00% <0.00%> (ø)
... and 2 more

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

@vaszig vaszig requested a review from jiromaykin March 9, 2023 10:33
Copy link
Contributor

@jiromaykin jiromaykin left a comment

Choose a reason for hiding this comment

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

The problem here is really in the file:
src/open_inwoner/scss/components/Form/Form.scss

where this selector is causing multiple problems inside the project, it is selecting everything that is not a button (but...... almost everything is NOT a button anyway.... including DIV's and UL's.... 😳 so it's too generic):

&__control > .label *:not(.button) { width: 100%; }

So my proposal is to replace this with multiple slightly more specific selectors there, like this:

&__control > .label .input,
  &__control > .label .input[type='file'] {
    width: 100%;
    max-width: 100%;

    @media (min-width: 768px) {
      max-width: 50%;
    }
  }

I think i have selected all types of form elements here, that should still work in all other pages,
if not, new classes/attributes/wildcards can be added to it: I have also added a mobile-first/media-query
We need to make the forms look smaller on Desktop anyway, like in the design:
https://projects.invisionapp.com/share/UF134RWMWCK8#/screens/471596624
They can be 100% on mobile.
Not using '100vw' here because that may cause horizontal scrolling in some places.

@vaszig
Copy link
Contributor Author

vaszig commented Mar 10, 2023

@jiromaykin I have made a partial change-update according to your suggestion because with the change above (width for desktop at 50%) we had some weird results (see screenshots below). If you think we can fix it easily let's do it, otherwise feel free to create another task for updating generally the forms according to the design.

messages
plans
actions

@jiromaykin
Copy link
Contributor

@jiromaykin ...with the change above (width for desktop at 50%) we had some weird results (see screenshots below). [...] create another task for updating generally the forms according to the design.

Thanks for checking even more - I will add this task to my mobile-sprint list, so what you did here is fine for now.

Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

It'd be neat if the form inputs would render conditionally, so we'd only have the form render logic in once place (the if user.contact_type == "begeleider" in the form class and not also the template) but that's for another time.

@alextreme alextreme merged commit 494e79e into develop Mar 13, 2023
@alextreme alextreme deleted the feature/1160-add-profile-image-cropping branch March 13, 2023 13:08
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