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

Sort assets #7540

Merged
merged 26 commits into from
Aug 18, 2023
Merged

Sort assets #7540

merged 26 commits into from
Aug 18, 2023

Conversation

somebody1234
Copy link
Contributor

@somebody1234 somebody1234 commented Aug 10, 2023

Pull Request Description

Important Notes

The sort arrows have a slightly thicker border (changed from 2px to 2.14px), to remove the (very small) internal hole in the Figma design. It is possible to make the shape more accurate to the original design by using a polygon (or a path) that traces around the original outline instead, but I figured it's not worth spending the extra time on a fix that may not be correct.

ℹ️ The comparison function for sorting is quite complicated. I think this is the least intrusive change for now, but it is worth considering changing AssetsTable to store items internally as a tree instead, and do a preorder traversal to flatten it into an array when needed.

Screencast

screen-recorder-thu-aug-10-2023-16-41-50.webm

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@somebody1234 somebody1234 added the CI: No changelog needed Do not require a changelog entry for this PR. label Aug 10, 2023
@PabloBuchu PabloBuchu self-assigned this Aug 10, 2023
@somebody1234
Copy link
Contributor Author

ok so this is currently freezing when sorting new entries, since there is no folder with a key matching the asset's parent id. not sure what's the best way to solve this - there is no way to get the child's parent's key, since the asset table does not store the parent's current value (it has access to the initial value of the item, with the placeholder id - aka the key.)

@somebody1234
Copy link
Contributor Author

i think (?) this isn't really high priority or anything, so i think i'll try refactoring from an assets list to an assets tree here. it will be a lot of changes to have to test ( :( ) but unfortunately i think it may be the simplest solution

@somebody1234 somebody1234 marked this pull request as draft August 11, 2023 08:37
@somebody1234
Copy link
Contributor Author

somebody1234 commented Aug 11, 2023

post-refactor testing:

  • ✔️ adding entry to directory shows loading spinner while directory entries are loading
  • ✔️ generated name of new directory is one higher than all other visible children
  • ✔️ folder is inserted in correct position after sibling project has been inserted
  • ✔️ project is inserted in correct position after sibling folder has been inserted
  • ✔️ when deleting folders, an unrelated folder also disappeared for some reason. (logic bug - it reappears upon refresh) (fixed by new commit)
  • ✔️ sorting works properly and doesn't freeze, or put projects before folders, and works properly even with nested folders
  • ✔️ nested folders follow their parent folder when sorting

@somebody1234 somebody1234 marked this pull request as ready for review August 11, 2023 16:13
@somebody1234
Copy link
Contributor Author

Note: I think this refactor also makes it possible (or I guess, easy) to show the "this folder is empty" placeholder row instead of collapsing the folder.

However, it would only show after a delay, after all children have been actually deleted - this is why the current behavior is just to silently collapse the folder.

@somebody1234 somebody1234 mentioned this pull request Aug 15, 2023
5 tasks
@PabloBuchu
Copy link
Contributor

Sorting works fine but the logic needs a bit more improvements. The sort by name or modification date should mix the asset types if it results from sorting order. Now the group takes precedence

Screen.Recording.2023-08-18.at.12.19.17.mov

@somebody1234
Copy link
Contributor Author

fixed locally, but i just realized that sorting currently uses the initial item - so after renaming, it will still use the initial name, and after running a project, it will still use the creation date (or original "last modified" date) not the last modified date

@somebody1234
Copy link
Contributor Author

should be fixed now - also fixed creating new assets (e.g. "New Project" button)

@PabloBuchu PabloBuchu added the CI: Ready to merge This PR is eligible for automatic merge label Aug 18, 2023
@mergify mergify bot merged commit f3f2f06 into develop Aug 18, 2023
25 checks passed
@mergify mergify bot deleted the wip/sb/sort-assets branch August 18, 2023 15:47
mergify bot pushed a commit that referenced this pull request Aug 28, 2023
- Closes #7463
- Makes table header sticky
- Clips table body so it does not overlap table header

Other changes:
- Clip table header row so it does not overlap extra columns selector
- Hide extra columns selector on local backend (PM backend)
- Focus search bar if the keypress will type regular text
- Change row height from 40px to 32px
- Add "Share" button when the editor is open
- Make entire area of backend selector (Cloud <-> Local) clickable (previously, the padding was not clickable)
- Remove the up-arrow icon to open a project. Projects are now opened by switching to the project tab using the tab selector on the top left (or by double clicking the row).
- Fix opening newly created folder (previously its entries were appended to the end, rather than under the folder)
- Indent background of "name" column (the first column)
- Minor code style changes
- Add background back to "change password" modal (oops)
- Hide "open" context menu entry and show "stop" entry, when a project is currently running
- ℹ️ It might be a good idea to support the "open" action on directories as well, however it is difficult without the assets table refactor in #7540. As such, this functionality will not be added in this PR.
- Fix horizontal padding on "sign in" user menu entry
- Hide email/password validation when using oauth logins

More fixes for assets list:
- Project is inserted at start of list when there are no existing projects
- Project is inserted at start of children when there are no existing children
- Deleting a folder collapses it (hides its descendants)
- Adding children to a newly created folder puts them at the correct depth, rather than depth 1

# Important Notes
None
mergify bot pushed a commit that referenced this pull request Feb 26, 2024
- Remove unused `forceCreate` parameter from backend requests
- Also undo bad change to behavior of sorting assets:
- #7540 (comment)
- Fix enso-org/cloud-v2#910
- Closing a project now doesn't close other projects.
- Unable to properly test, as I cannot get the second project to open
- Fix enso-org/cloud-v2#908
- Use the `download` module that already exists to download the app...
- Add correct arrow icon for collapsing/expanding folders
- Turns out this icon had always existed, and I just didn't see it for the longest time
- Hide irrelevant parts of search bar on Local Backend
- The row of labels
- `no:`, `has:`, `label:`, `-label:`, `description:`, `-description:` (and their corresponding autocomplete entries)
- Also change text to say "Type to search for projects." because the other asset types mentioned do not (and will never) exist on the Local Backend.
- Fix search bar eating up `"` and ` ` keypresses
- Fix icons for Data Links and Secrets being swapped in `AssetIcon`

# Important Notes
- The fix for enso-org/cloud-v2#908 has not been tested on Safari (I don't have a macOS machine.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants