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

Use notification service in gmf authentication directive #1245

Merged

Conversation

adube
Copy link
Contributor

@adube adube commented May 18, 2016

This PR introduces the usability of the ngeo.Notification service within the gmf.Authentication directive. The message are displayed where they used to be.

Todo

  • review

Live example

Try:

  • login with bad credentials
  • change password with bad credentials

in this example:

@sbrunner
Copy link
Member

Two comments not directly related:

  • The message « Could not connect. » is not user friendly... shouldn't be something like « Wrong user name or password. »?
  • The empty field message should use the same kind of message...

Also that's looks good :-)

@adube
Copy link
Contributor Author

adube commented May 18, 2016

The message « Could not connect. » is not user friendly... shouldn't be something like « Wrong user name or password. »?

This is out of scope of this PR, indeed.

The empty field message should use the same kind of message...

I'm not sure where those messages come from. The browser ? Boostrap ? Angular ? I'd like to have help regarding this if we're to change this as well. They are automatically shown because the fields are required. Should we remove the requirement ?

@sbrunner
Copy link
Member

  1. => Could not connect... #1250
  2. From the browser because we set the HTML attribute required="required"

@adube
Copy link
Contributor Author

adube commented May 18, 2016

Okay, thanks.

Should this be covered in this PR ? If so, what's the plan. Do we remove required="required" ?

@sbrunner
Copy link
Member

Should this be covered in this PR ?

As you want...

If so, what's the plan. Do we remove required="required" ?

Yes :-), add add a custom message :-)

@adube
Copy link
Contributor Author

adube commented May 18, 2016

I'll make an other PR.

@sbrunner
Copy link
Member

@fredj can we merge this pull request?

@fredj
Copy link
Member

fredj commented May 19, 2016

yes, merging

@fredj fredj merged commit 10761b6 into camptocamp:master May 19, 2016
@adube adube deleted the notification-on-authentication-failure branch May 19, 2016 11:58
@sbrunner sbrunner added this to the Older milestone Aug 23, 2019
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.

None yet

3 participants