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

Root Workspace Sort Dropdown (Alphabetical) #3481

Open
wants to merge 53 commits into
base: develop
Choose a base branch
from

Conversation

csjasonl
Copy link

Description

Added dropdown box in the workspace directory list to sort root workspaces alphabetically (ascending/descending).
Addresses enhancement #3057 which was concerned with same-name root directories of different parent folders not being
ordered alphabetically.

Image of the dropdown
image

image

image

Changes

  • Added .ts file 'source/win-main/file-manager/util/sort-directories.ts' containing the sort function.
  • Modified .vue file 'source/win-main/file-manager/file-tree.vue' to incorporate GUI dropdown and function usage.
    small dropdown textbox is positioned next to the Workspaces header, and scales with the sidebar and window size.
  • Added unit test file 'test/sort-directories.spec.ts' to test function use cases. Runs alongside other unit tests.

Additional information

- The development team is doing this as part of a university course, and would appreciate any feedback

  • Translations are required for the new phrases:
    ** gui.sort_by_option
    ** gui.alphabetical_asc_option
    ** gui.alphabetical_desc_option
    placeholder text is available until translation files are updated
  • The default behaviour for ordering root directories is still available as the default "Sort By" option.
  • Other ordering types (e.g. by access or creation date) can be more easily implemented in future.
  • Layered parent same-name directories aren't shown, but they may still sorted (e.g. Directory A/X/Root & B/X/Root are shown as Root (X)).

Tested on: Windows 10 Home 64-bit (10.0 Build 19044), Ubuntu 20.04.4 LTS, macOS BigSur 11.6

@boring-cyborg
Copy link

boring-cyborg bot commented May 18, 2022

Thank you for opening your first PR! 🎉 We are very happy and would like to thank you very much for your contribution. If everything checks out, we'll make sure to review the PR as soon as possible and give feedback. In the meantime, to make the reviewing process as fast as possible, you can help us by checking the following things:

  • Did you follow the JSStandard coding style? - Did you comment everywhere where the necessity of a piece of code or the
    way it was implemented is not immediately obvious?
  • Did you attempt to stick as much to current coding habits as possible?
    (Note that this does not apply to pieces of code where we ourselves
    obviously violated good coding practices, which unfortunately happens
    sometimes. But please indicate this in your PR so that we know what you
    rectified!)

Furthermore, make sure that the linter does not complain, which will check your code on every new commit. If the linter task fails, make sure to run yarn lint locally and check the file eslint_report.htm which will tell you precisely what went wrong.
Stay sharp, and thanks again!

@csjasonl csjasonl marked this pull request as ready for review May 18, 2022 10:31
Move new feature description to latest unreleased develop
Reverted hard change
reverted committed changes to current develop branch
@nathanlesage
Copy link
Member

Thank you very much for this PR. Thus far, it looks good from a code perspective, but I think there are a few conceptual issues we might want to discuss before moving forward.

  1. Should the sort really only happen in the renderer, or wouldn't it be better to sort the paths directly in the configuration to begin with? That way, if we ever decide to display the root folders (this is a 100% certainty) we won't lose that ability just because we forget to implement your sorter.
  2. Should this really be done using a dropdown inside the workspaces header? On macOS that would be a no-go. Plus, chances are that people only would want to set this setting once, which would make the preferences a more suitable place for this setting.
    • 2a: If we choose to stick to the dropdown, I recommend utilizing the select Vue component in the common directory, which already implements native styling and which saves you from modifying the CSS too much
    • 2b: Having the default option as an "Don't sort" sounds bogus to me and I think this might confuse people, so we should choose another wording for that
  3. There is an Intl.compare function, which I think the file sorting algorithm of the FSAL utilizes. This way, we can make sure that even non-Western scripts are sorted appropriately. Maybe you can even utilize that sorter for your purposes. Would keep the code DRY and mitigate potential bugs. We can move that utility function outside of the FSAL, if necessary
  4. Lastly, I just remembered that I think the roots are already sorted to some degree, so this functionality here would potentially interfere with that. We need to check that.

@kyaso What are your thoughts on this?

@kyaso
Copy link
Collaborator

kyaso commented May 22, 2022

  1. [...] we won't lose that ability just because we forget to implement your sorter.

I have no particular opinion on point 1, but if the quoted part is a potential issue, I would agree with moving the sort to the configuration (@nathanlesage: is "configuration" in this context equivalent to the "store.state.fileTree"?)

  1. Should this really be done using a dropdown inside the workspaces header?

I would also say that the preferences might be a more suitable place for that.

  1. There is an Intl.compare function, which I think the file sorting algorithm of the FSAL utilizes. This way, we can make sure that even non-Western scripts are sorted appropriately.

I agree with this one.

  1. Lastly, I just remembered that I think the roots are already sorted to some degree, so this functionality here would potentially interfere with that. We need to check that.

Unfortunately, I'm not too familiar with that part of the codebase. But yeah, I guess it might be worth investigating.

@nathanlesage
Copy link
Member

Yes, I meant to implement it in the config provider, or in the FSAL. One of those two places already implements some form of root sorting, I think it should be the FSAL.

@stale
Copy link

stale bot commented Aug 13, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale This label indicates that this issue might be automatically closed soon. label Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale This label indicates that this issue might be automatically closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants