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

[#1898] Design profile page for businesses #875

Merged
merged 4 commits into from
Dec 19, 2023

Conversation

pi-sigma
Copy link
Contributor

@pi-sigma pi-sigma commented Dec 4, 2023

Taiga #1895

@pi-sigma pi-sigma force-pushed the feature/1898-profile-page-business branch 3 times, most recently from dfa70fa to 8cb7d5f Compare December 4, 2023 12:52
@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (59dc8af) 92.81% compared to head (fc0d9d5) 92.82%.
Report is 2 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #875   +/-   ##
========================================
  Coverage    92.81%   92.82%           
========================================
  Files          803      803           
  Lines        27531    27559   +28     
========================================
+ Hits         25553    25581   +28     
  Misses        1978     1978           

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

@pi-sigma pi-sigma marked this pull request as ready for review December 4, 2023 13:09
@pi-sigma pi-sigma requested a review from jiromaykin December 4, 2023 13:10
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.

Something that needs extra checking: is the "None" value removed from all potentially empty fields in case they're empty? Needs to be an empty string and it preferably should not take up any space in the frontend.

src/open_inwoner/templates/pages/profile/me.html Outdated Show resolved Hide resolved
src/open_inwoner/templates/pages/profile/me.html Outdated Show resolved Hide resolved
src/open_inwoner/templates/pages/profile/me.html Outdated Show resolved Hide resolved
src/open_inwoner/templates/pages/profile/me.html Outdated Show resolved Hide resolved
@pi-sigma pi-sigma force-pushed the feature/1898-profile-page-business branch 5 times, most recently from 2f21b84 to fc0d9d5 Compare December 12, 2023 08:14
@jiromaykin jiromaykin self-assigned this Dec 12, 2023
@jiromaykin jiromaykin marked this pull request as draft December 12, 2023 17:01
@jiromaykin jiromaykin force-pushed the feature/1898-profile-page-business branch 2 times, most recently from f2e0583 to 28c4aaf Compare December 18, 2023 11:41
@jiromaykin jiromaykin force-pushed the feature/1898-profile-page-business branch from 28c4aaf to 69104a4 Compare December 18, 2023 11:50
@jiromaykin jiromaykin marked this pull request as ready for review December 18, 2023 14:04
@jiromaykin jiromaykin self-requested a review December 18, 2023 14:04
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.

I am approving this for the front-end at least.

Copy link
Contributor

@stevenbal stevenbal left a comment

Choose a reason for hiding this comment

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

Looks good, I do have some questions (might be the case that this is by design, but I just want to double check):

  • There is a bewerk button above voorkeur voor meldingen, bus only ontvang meldingen over can be edited and communicatiekanaal cannot be edited?
    image

  • bekijk alle gegevens is present in the design but I don't think it's implemented? image

  • I couldn't find email anywhere in the profile?

src/open_inwoner/accounts/tests/factories.py Outdated Show resolved Hide resolved
src/open_inwoner/templates/pages/profile/me.html Outdated Show resolved Hide resolved
@jiromaykin
Copy link
Contributor

  • bekijk alle gegevens is present in the design but I don't think it's implemented?

That's a good question @stevenbal ; I am not sure there is actually any more data to show for Bedrijven, so perhaps @pi-sigma can answer this.

  • I couldn't find email anywhere in the profile?

In future there is the wish to also add SMS/textmessaging but this functionality doesn't exist yet; not sure if this is too much of a problem but perhaps it does feel like people are given a choice to edit something here and then they are let down because they actually can't.
What do you think @alextreme ?

@alextreme
Copy link
Member

  • I couldn't find email anywhere in the profile?

In future there is the wish to also add SMS/textmessaging but this functionality doesn't exist yet; not sure if this is too much of a problem but perhaps it does feel like people are given a choice to edit something here and then they are let down because they actually can't. What do you think @alextreme ?

I discussed this and simply stating E-mail as per the design is fine.

@pi-sigma
Copy link
Contributor Author

@jiromaykin We could retrieve and display various companay data via the kvk api, but this is not implemented and wasn't part of the spec, as far as I remember. If this is desired, we should create a new ticket and discuss what data should be retrieved.

@stevenbal You're right about both points; will fix and push final changes.

@stevenbal
Copy link
Contributor

Looks good to me 👍

@alextreme alextreme merged commit 9cac5a4 into develop Dec 19, 2023
14 checks passed
@alextreme alextreme deleted the feature/1898-profile-page-business branch December 19, 2023 13:12
alextreme added a commit that referenced this pull request Jan 9, 2024
…siness

[#1898] Design profile page for businesses
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