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

[#9207] Sort school names for userMapData.json #9269

Merged
merged 6 commits into from Dec 15, 2018

Conversation

monmanuela
Copy link
Contributor

@monmanuela monmanuela commented Dec 12, 2018

Fixes #9207

PR Checklist

Ensure that you have:

  • Read and understood our PR guideline: https://github.com/TEAMMATES/teammates/blob/master/docs/process.md#step-4-submit-a-pr
    • Added the issue number to the "Fixes" keyword above
    • Titled the PR as specified in the abovementioned document
  • Made your changes on a branch other than master and release
  • Gone through all the changes in this PR and ensured that:
    • They addressed one (and only one) issue
    • No unintended changes were made
  • Run and passed static analysis: ./gradlew lint and npm run lint
  • Added/updated tests, if changes in functionality were involved
  • Added/updated documentation to public APIs (classes, methods, variables), if applicable

Outline of Solution

Following @wkurniawan07 's suggestion here, I used javascript's Array.sort() method to reorder the school names for each country.

Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

While array.sort() works and is the most straightforward way to sort items, it is also taking into account case sensitivity, which results in many out-of-expectation ordering. Can you tweak your sorting script a bit to make it case insensitive?

@monmanuela
Copy link
Contributor Author

Done!

Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

Also, can you put the script you used at the root project folder? Preferable if the script updates the file in-place, i.e. after running node sort-usermap (or whatever name you used) I only need to commit and not copy-pasting.

src/main/webapp/data/userMapData.json Outdated Show resolved Hide resolved
@monmanuela
Copy link
Contributor Author

Done for both. Should I document about the script somewhere?

@wkurniawan07
Copy link
Member

@monmanuela No need to do anything with the script for now; it's good enough where it is.

Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution!

@wkurniawan07 wkurniawan07 added the s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging label Dec 15, 2018
@wkurniawan07 wkurniawan07 merged commit ea2ef33 into TEAMMATES:master Dec 15, 2018
@xpdavid xpdavid added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Dec 17, 2018
@xpdavid xpdavid added this to the V6.16.0 milestone Dec 17, 2018
jacoblipech pushed a commit to jacoblipech/teammates that referenced this pull request Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort school names for userMapData.json
3 participants