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

feat: improve validation and UI/UX #187

Merged
merged 1 commit into from
Oct 28, 2021
Merged

feat: improve validation and UI/UX #187

merged 1 commit into from
Oct 28, 2021

Conversation

bfabio
Copy link
Member

@bfabio bfabio commented May 26, 2021

  • fix: centered layout in home page

  • fix: don't mention E.164 in phone number validation
    E.164 is unknown to most users.
    Besides, the validation never checked for it anyway and the
    majority of past onboardings were performed with no international
    prefix.

  • feat: accept spaces in phone numbers to lower the chance for
    error and improve the UX.

  • feat: better PA picker (Fixes Rethink the PA picker widget #158)
    The PA picker in home page does not flicker when typing,
    it's properly aligned and doesn't rely on the readonly support
    fields anymore.

    The results are shown in a more intuitive way.

    Also, the input is debounced and makes less calls to Elasticsearch.

  • fix: ability to edit the form on error (Fixes Can't continue the compilation on error #159)
    When the server side validation fails, the user can now fix what's
    wrong right there instead of starting over.

  • refactor: simplify the validators (Fixes Refactor the validation logic #138)

  • feat: code hosting URL validation
    The validation now rejects non HTTPS URLs and URLs that don't
    look like a code hosting organization.

    (ie. https://github.com/foobar/repo will be rejected, but
    https://github.com/foobar won't)

@bfabio
Copy link
Member Author

bfabio commented May 26, 2021

The changes are somewhat interconnected by I can split them up if the single commit is a problem.

* fix: centered layout in home page

* fix: don't mention E.164 in phone number validation
  E.164 is unknown to most users.
  Besides, the validation never checked for it anyway and the
  majority of past onboardings were performed with no international
  prefix.

* feat: accept spaces in phone numbers to lower the chance for
  error and improve the UX.

* feat: better PA picker (Fixes italia#158)
  The PA picker in home page does not flicker when typing, provides
  loading feedback, it's properly aligned and doesn't rely on the
  readonly support fields anymore.

  The result are shown in a more intuitive way.

  Also, the input is debounced and makes less calls to Elasticsearch.

* fix: ability to edit the form on error (Fixes italia#159)
  When the server side validation fails, the user can now fix what's
  wrong right there instead of starting over.

* refactor: simplify the validators (Fixes italia#138)

* feat: code hosting URL validation
  The validation now rejects non HTTPS URLs and URLs that don't
  look like a code hosting organization.

  (ie. https://github.com/foobar/repo will be rejected, but
  https://github.com/foobar won't)
* @param {string} ipa
* @param {string} url
*/
function checkDups(ipa, url) {
function isAlreadyOnboarded(ipa, url) {
Copy link
Member

Choose a reason for hiding this comment

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

@bfabio @sebbalex Shouldn't we add the "substring" check here?

Copy link
Member Author

@bfabio bfabio Oct 27, 2021

Choose a reason for hiding this comment

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

@libremente the regex should already be checking for an organization, making it impossible to add a single repo

Copy link
Member

Choose a reason for hiding this comment

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

That regex is fine, however we still have the org vs user issue, since that checks only the form of the URL and not the substance, right?

Copy link
Member Author

@bfabio bfabio Oct 27, 2021

Choose a reason for hiding this comment

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

@libremente That's correct but for us to tell those two cases apart we have to use the code hostings' APIs (ie, we can address that in a separate patch).

@bfabio bfabio merged commit 5fd049d into italia:master Oct 28, 2021
@bfabio bfabio deleted the refactor branch July 7, 2022 07:38
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.

Can't continue the compilation on error Rethink the PA picker widget Refactor the validation logic
3 participants