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 a built-in JS function localeCompare() to compare strings #1141

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

kirisakow
Copy link
Contributor

This PR is a proposal for using a built-in JavaScript function localeCompare() for strings sorting. If you read the documentation, you'll see it does exactly what was previously implemented by hand.

Pros:

  • it is a built-in function of the JavaScript API;

  • s1.localeCompare(s2, 'your-locale') looks cleaner and more legible than nested ternary operators that used to be there;

  • it is more straightforward at handling numeric strings: no need to parseInt(s) or parseFloat(s) before comparison, just s1.localeCompare(s2, 'your-locale', {numeric: true});

@elzody
Copy link
Contributor

elzody commented Jun 15, 2024

Hey there, this looks pretty good, other than for me it isn't working for columns which are not strings, so I think it would need adjusted for other column types is my guess.

See screenshot here

image

@kirisakow
Copy link
Contributor Author

@elzody Thank you.

Changes undone for files

  • src/shared/components/ncTable/mixins/columnsTypes/number.js
  • src/shared/components/ncTable/mixins/columnsTypes/numberProgress.js
  • src/shared/components/ncTable/mixins/columnsTypes/numberStars.js
  • src/shared/components/ncTable/mixins/columnsTypes/selection.js

Side question: What's your NC setup for development and testing?

Lately I've been having annoying misbehavior in my local NC developer instance I had spent a few hours to setup... So I've started thinking I should scrap it and go the docker-compose way. I'd appreciate if you guys @elzody @enjeck could suggest me an all-ready docker-compose.yml file as well as any steps to set it up... if you have such knowledge. (Sure, I've seen there are lots of images in nextcloud/docker-ci repository, but I can't wrap my head around it all on my own...)

@elzody
Copy link
Contributor

elzody commented Jun 15, 2024

@kirisakow My setup, believe it or not, is also a bit messy. I need to take the time to improve the workflow a bit, but generally I use the standalone container and just bind mount the directories of the apps I am testing/developing for into the container. You can find more info about the standalone containers here, and a sample of my Docker compose file is like so:

services:
  nextcloud:
    image: ghcr.io/juliushaertl/nextcloud-dev-php82:latest
    container_name: nextcloud
    restart: unless-stopped
    ports:
      - "7128:80"
    environment:
        - SERVER_BRANCH=master
    volumes:
        # Here I just bind mount the apps, including server
        # If you don't do this, it's fine, it will auto clone it, etc. based on SERVER_BRANCH above
        - type: bind
          source: server        # Path to where I have server repo cloned
          target: /var/www/html

        - type: bind
          source: tables        # Path to where I have tables repo cloned
          target: /var/www/html/apps-extra/tables

Isn't the best, but works for me. The only part of the compose file I left out was that related to the Collabora container, not sure if you are gonna be using that at all.

I will give another review here when I get the chance :)

Copy link
Contributor

@elzody elzody left a comment

Choose a reason for hiding this comment

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

Gave it a test, and it works nicely. Looks much more readable than the weird ternary operators before.

I am not quite sure what is up with the DCO check failing, but I suspect it has something to do with the documentation thing included in your first commit. Try doing an interactive rebase and see if you can alter that message to remove it, and we will see if that fixes it

kirisakow and others added 2 commits June 18, 2024 19:09
Signed-off-by: Kiril Isakov <kirix.isakov@gmail.com>
Signed-off-by: Kiril Isakov <kirix.isakov@gmail.com>
@kirisakow kirisakow force-pushed the use-built-in-js-func-localeCompare branch from b255c47 to 5eef3a3 Compare June 18, 2024 17:12
@kirisakow
Copy link
Contributor Author

kirisakow commented Jun 18, 2024

I am not quite sure what is up with the DCO check failing, but I suspect it has something to do with the documentation thing included in your first commit. Try doing an interactive rebase and see if you can alter that message to remove it, and we will see if that fixes it

You are right. My original commit message was split into 3 lines (with the URL on the 2nd line), but DCO seemingly wants no more than one newline break, with the sign-off being on the 2nd line. 🤷‍

@elzody elzody merged commit 57f42a0 into nextcloud:main Jun 18, 2024
45 checks passed
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants