nextcloud / tables

🍱 Nextcloud tables app
https://apps.nextcloud.com/apps/tables
GNU Affero General Public License v3.0
133 stars 21 forks source link

use a built-in JS function `localeCompare()` to compare strings #1141

Closed kirisakow closed 1 week ago

kirisakow commented 2 weeks ago

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:

elzody commented 2 weeks ago

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](https://github.com/nextcloud/tables/assets/23369449/46d5a9f1-bffb-4d09-a891-04767e0fa6a1)
kirisakow commented 1 week ago

@elzody Thank you.

Changes undone for files

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 commented 1 week ago

@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 :)

kirisakow commented 1 week ago

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. 🤷‍

github-actions[bot] commented 1 day ago

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.)