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

Issue 662 area order and sorting #696

Merged
merged 8 commits into from
May 21, 2023
Merged

Issue 662 area order and sorting #696

merged 8 commits into from
May 21, 2023

Conversation

tedgeving
Copy link
Contributor

Work In Progress Addresses part of the following Issue
#662

DND Library has been (https://github.com/atlassian/react-beautiful-dnd) added to the Area edit page
https://github.com/OpenBeta/open-tacos/blob/develop/src/components/edit/AreaCRUD.tsx#L43

@tedgeving tedgeving self-assigned this Feb 9, 2023
@vercel
Copy link

vercel bot commented Feb 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
open-tacos ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 20, 2023 5:37pm

@vnugent
Copy link
Contributor

vnugent commented Feb 9, 2023

Nice! Can you check the preview build? The list is showing up twice
https://open-tacos-git-issue-662-area-order-openbeta-dev.vercel.app/crag/793d57d9-1f16-5457-8810-cc4e05e07421

@tedgeving
Copy link
Contributor Author

@vnugent Removed the duplicate listing.

@vnugent
Copy link
Contributor

vnugent commented Feb 15, 2023

@tedgeving what do you think if we merge this PR now and create a follow up one when the backend functionality is done?

@tedgeving
Copy link
Contributor Author

@vnugent that works for me.

@tedgeving tedgeving closed this Feb 15, 2023
@vnugent
Copy link
Contributor

vnugent commented Mar 22, 2023

@tedgeving any chance you can revive this PR?

If you can rebase the PR with develop and get the drag-n-drop UI working again, I can finish the backend integration part.

@vnugent vnugent reopened this Mar 22, 2023
@vnugent vnugent temporarily deployed to Preview March 22, 2023 15:48 — with GitHub Actions Inactive
@tedgeving
Copy link
Contributor Author

tedgeving commented Mar 22, 2023 via email

@glassbead0
Copy link

I love this feature or ordering areas! Just tracking down the current progress and I see the backend work is done, and looks like the frontend is still a WIP.

I'm curious if it makes sense to implement the area sorting UI using the same CSV Power Editor that is used for route sorting (cut/paste areas in the order you want), instead of a drag and drop. While I think drag and drop is just fine, it does create inconsistency. I have limited time to help with this (and no experience yet with the tech stack), but if I do get time I will start looking at code to see if I can help at all.

@zichongkao zichongkao temporarily deployed to Preview May 17, 2023 19:58 — with GitHub Actions Inactive
@zichongkao zichongkao temporarily deployed to Preview May 17, 2023 21:39 — with GitHub Actions Inactive
@zichongkao
Copy link
Contributor

zichongkao commented May 17, 2023

@vnugent Two outstanding problems:

  1. In testing, I'm hitting the duplicate index error for leftRightIndex. I think it's because I'm sending [{areaId: .., leftRightIndex: 0}, {areaId: .., leftRightIndex: 1}, .., {areaId: .., leftRightIndex: <# of childareas -1>}] and when we try to set the leftRightIndex to be 0 for the first item, there is already another entry in the db with leftRightIndex 0, so throws the error even though later on, that item's leftRightIndex will replaced by a different entry. I could reset everything to -1 and then reassign, but it's not that elegant. Curious if you have a better approach in mind.
  2. The drag and drop works fine in a single column, but you can't drag things across two columns. It doesn't detect the x coordinate, only the y coordinate, so it thinks you're dropping it into the leftmost column. Basically I think react-beautiful-dnd only supports either horizontal or vertical lists. When we have two columns, we need both directions.
    Screen Recording 2023-05-17 at 4 02 09 PM

@tedgeving
Copy link
Contributor Author

tedgeving commented May 17, 2023 via email

@vnugent
Copy link
Contributor

vnugent commented May 18, 2023

Unfortunately, wrapped list isn't supported by the DnD library. See this long discussion: atlassian/react-beautiful-dnd#316

An alternative? https://github.com/clauderic/dnd-kit

@zichongkao
Copy link
Contributor

zichongkao commented May 18, 2023

Yup -- that's what I landed on too. Seems pretty mature and well-supported. Now we can handle multiple columns:
Screen Recording 2023-05-18 at 1 06 41 AM

@zichongkao
Copy link
Contributor

Next up, we'd have to update src/components/crag/cragTable.tsx to do the same there.

@zichongkao
Copy link
Contributor

@zichongkao zichongkao temporarily deployed to Preview May 19, 2023 21:31 — with GitHub Actions Inactive
@zichongkao zichongkao requested a review from vnugent May 19, 2023 22:18
@zichongkao zichongkao changed the title WIP Issue 662 area order and sorting Issue 662 area order and sorting May 19, 2023
@zichongkao zichongkao temporarily deployed to Preview May 20, 2023 05:17 — with GitHub Actions Inactive
@zichongkao zichongkao temporarily deployed to Preview May 20, 2023 06:12 — with GitHub Actions Inactive
@@ -117,7 +117,7 @@ export default function CragSummary ({ area, history }: CragSummaryProps): JSX.E
* Initially set to the childAreas prop coming from Next build, the cache
* may be updated by the users in the AreaCRUD component.
*/
const [childAreasCache, setChildAreasCache] = useState(childAreas)
const [childAreasCache, setChildAreasCache] = useState(sortByLeftRightIndex(childAreas))
Copy link
Contributor

Choose a reason for hiding this comment

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

One question worth discussing is whether we should be sorting in the frontend or backend. You could argue that frontend is better if we want to allow dynamic sorting (L->R, Alphabetical, Most children) one day, so the backend result remain invariant and can be cached. Backend sorting just simplifies frontend code a bunch especially in the case where we always sort L->R.

I realize that for areas, L->R sorting doesn't make too much sense because areas aren't lined up on a wall like climbs. So the main purpose of the sorting here is just a stable ordering. If stability is what we care about, then perhaps no point enabling multiple sort orders for areas, which would lead to backend sorting being better.

For now I'm just following the design pattern of climbs, and doing sorting on the frontend.

Copy link
Contributor

Choose a reason for hiding this comment

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

All good points. Agreed we should sort in the backend. Can you create a backend issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

OpenBeta/openbeta-graphql#294 Sounds good. I can work on this next.

@@ -4,9 +4,28 @@ import { AreaSummaryType } from '../crag/cragSummary'
import { DeleteAreaTrigger, AddAreaTrigger, AddAreaTriggerButtonMd, AddAreaTriggerButtonSm, DeleteAreaTriggerButtonSm } from './Triggers'
import { AreaEntityIcon } from '../EntityIcons'
import NetworkSquareIcon from '../../assets/icons/network-square-icon.svg'
import React, { useState } from 'react'
import useUpdateAreasCmd from '../../js/hooks/useUpdateAreasCmd'
import { useSession } from 'next-auth/react'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not super important. I usually place library imports at the top and then our own stuff. ts-standard doesn't enforce this rule but I think it helps you quickly gain some context when scanning the import section.

https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed!

package.json Show resolved Hide resolved
@zichongkao zichongkao temporarily deployed to Preview May 20, 2023 17:35 — with GitHub Actions Inactive
@vnugent vnugent merged commit 6759f2c into develop May 21, 2023
7 checks passed
@vnugent vnugent deleted the issue-662-area-order branch May 21, 2023 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants