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

Ensure givenName is not required for using Profile #205

Merged
merged 2 commits into from
Oct 12, 2021

Conversation

MKodde
Copy link
Member

@MKodde MKodde commented Oct 11, 2021

Not having the GivenName in the released attribute set would halt
profile in the IntroductionController. Effectively showing an error
right after logging in to Profile.

This chane makes the use of the givenName attrib optional. An additional
change might be required ot make the UX more fluent as the header
element is now only stating 'Hi !' when logging in.

One step at a time!

https://www.pivotaltracker.com/story/show/179882084

Not having the GivenName in the released attribute set would halt
profile in the IntroductionController. Effectively showing an error
right after logging in to Profile.

This chane makes the use of the givenName attrib optional. An additional
change might be required ot make the UX more fluent as the header
element is now only stating 'Hi !' when logging in.

One step at a time!

https://www.pivotaltracker.com/story/show/179882084
@MKodde MKodde requested a review from Badlapje October 11, 2021 08:20
Copy link

@Badlapje Badlapje left a comment

Choose a reason for hiding this comment

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

A small nitpick, but otherways great work!

There is a security vulnerability atm though that can be resolved with npm audit fix.

@@ -9,8 +9,11 @@
{% endblock %}

{% block content %}
{% if userName != false %}

Choose a reason for hiding this comment

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

This can probably be shortened to if userName

Copy link
Member Author

Choose a reason for hiding this comment

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

That's primo advice!

For PT I've left the current setup where we have the welcome to the
profile page text. For NL I updated the title according to the
suggestion provided by Arnout.
@MKodde MKodde force-pushed the bugfix/givenName-not-required branch from 801d162 to 28b41c2 Compare October 12, 2021 08:18
@MKodde
Copy link
Member Author

MKodde commented Oct 12, 2021

The security warning is fixed in one of the other PR's
Moving forward with this

@MKodde MKodde merged commit 0ad2c7c into develop Oct 12, 2021
@MKodde MKodde deleted the bugfix/givenName-not-required branch October 12, 2021 08:19
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.

2 participants