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

Add username #1133

Merged
merged 34 commits into from
Jun 17, 2021
Merged

Add username #1133

merged 34 commits into from
Jun 17, 2021

Conversation

tanmoyAtb
Copy link
Contributor

closes #1091

@render
Copy link

render bot commented Jun 14, 2021

@render
Copy link

render bot commented Jun 14, 2021

@github-actions github-actions bot added the Type: Feature Added to PRs to identify that the change is a new feature. label Jun 14, 2021
@tanmoyAtb tanmoyAtb changed the title add username Add username Jun 15, 2021
@tanmoyAtb tanmoyAtb requested review from Tbaut, FSM1 and RyRy79261 June 15, 2021 16:06
@tanmoyAtb tanmoyAtb marked this pull request as ready for review June 15, 2021 16:06
@tanmoyAtb tanmoyAtb added the Status: Review Needed 👀 Added to PRs when they need more review label Jun 15, 2021
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Working well but I'm getting some 404 on https://stage.imploy.site/api/v1/user/lookup when typing something (although the lookup does work).

packages/common-components/src/TextInput/TextInput.tsx Outdated Show resolved Hide resolved
<FormikTextInput
placeholder={t`First name`}
name="firstName"
size="medium"
hideLabel={true}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't check deeper but can't we have empty label rather?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the label is actually optional, but the FormikTextInput passes the field.name and I had no way of removing that since its being used else where in code.

@Tbaut Tbaut added Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. and removed Status: Review Needed 👀 Added to PRs when they need more review labels Jun 16, 2021
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@CLAassistant
Copy link

CLAassistant commented Jun 16, 2021

CLA assistant check
All committers have signed the CLA.

tanmoyAtb and others added 4 commits June 17, 2021 00:29
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@lgtm-com
Copy link

lgtm-com bot commented Jun 16, 2021

This pull request fixes 1 alert when merging d666072 into f1852e9 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

tanmoyAtb and others added 11 commits June 17, 2021 00:51
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@tanmoyAtb
Copy link
Contributor Author

Working well but I'm getting some 404 on https://stage.imploy.site/api/v1/user/lookup when typing something (although the lookup does work).

Yes, the lookup api responds with 404 when it doesn't find a username we are sending,
not sure if thats a standard behaviour but it works.

@tanmoyAtb tanmoyAtb requested a review from Tbaut June 16, 2021 18:59
@lgtm-com
Copy link

lgtm-com bot commented Jun 16, 2021

This pull request fixes 1 alert when merging 9efa618 into f1852e9 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 16, 2021

This pull request fixes 1 alert when merging 2e4e72f into 2f3d1eb - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@Tbaut
Copy link
Collaborator

Tbaut commented Jun 17, 2021

api responds with 404 when it doesn't find a username we are sending,
not sure if thats a standard behaviour but it works.

ok a 404 is definitely not a standard for an expected answer, this is showing red things all over (network and console tab), it is handled as an error in the code and in the browser. I guess it should be a 200 with null or something. I'll check with the api team

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

👍 nice one! Just the cla signing is missing 🤷‍♂️

@tanmoyAtb
Copy link
Contributor Author

api responds with 404 when it doesn't find a username we are sending,
not sure if thats a standard behaviour but it works.

ok a 404 is definitely not a standard for an expected answer, this is showing red things all over (network and console tab), it is handled as an error in the code and in the browser. I guess it should be a 200 with null or something. I'll check with the api team

Yes a 200 with null, sounds right!

@tanmoyAtb tanmoyAtb merged commit 0382ee6 into dev Jun 17, 2021
@tanmoyAtb tanmoyAtb deleted the feat/add-username-1091 branch June 17, 2021 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. Type: Feature Added to PRs to identify that the change is a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Nickname field to the profile overview
5 participants