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

Chore: Tidy Drag & Drop Code for Consistency #416

Closed
3 tasks done
nelsonic opened this issue Sep 7, 2023 · 2 comments · Fixed by #419
Closed
3 tasks done

Chore: Tidy Drag & Drop Code for Consistency #416

nelsonic opened this issue Sep 7, 2023 · 2 comments · Fixed by #419
Assignees
Labels
chore a tedious but necessary task often paying technical debt elixir Pull requests that update Elixir code help wanted If you can help make progress with this issue, please comment! priority-2 Second highest priority, should be worked on as soon as the Priority-1 issues are finished T2h Time Estimate 2 Hours tech-debt A feature/requirement implemented in a sub-optimal way & must be re-written technical A technical issue that requires understanding of the code, infrastructure or dependencies

Comments

@nelsonic
Copy link
Member

nelsonic commented Sep 7, 2023

At present the code in assets/js/app.js, lib/app_web/live/app_live.html.heex and lib/app_web/live/app_live.ex has inconsistent naming of events and variables ... 🙃

e.g:

mvp/assets/js/app.js

Lines 35 to 58 in a7408a1

this.el.addEventListener("remove-highlight", e => {
hook.pushEventTo("#items", "removeHighlight", {id: e.detail.id})
// console.log('remove-highlight', e.detail.id)
})
this.el.addEventListener("dragoverItem", e => {
// console.log("dragoverItem", e.detail)
const currentItemId = e.detail.currentItem.id
const selectedItemId = e.detail.selectedItemId
if( currentItemId != selectedItemId) {
hook.pushEventTo("#items", "dragoverItem", {currentItemId: currentItemId, selectedItemId: selectedItemId})
itemId_to = e.detail.currentItem.dataset.id
}
})
this.el.addEventListener("update-indexes", e => {
const item_id = e.detail.fromItemId
const list_ids = get_list_item_cids()
console.log("update-indexes", e.detail, "list: ", list_ids)
// Check if both "from" and "to" are defined
if(item_id && itemId_to && item_id != itemId_to) {
hook.pushEventTo("#items", "update_list_seq",
{seq: list_ids})
}

"remove-highlight" should be "remove_highlight" and "dragoverItem" should be "dragover_item" for consistency.

Todo

  • Review all the code in these files and the corresponding tests test/app_web/live/app_live_test.exs and update for consistency. 👩‍💻
  • Ensure that all tests still pass and functionality still works. ✅
  • Create PR and assign for review. :shipit:
@nelsonic nelsonic added help wanted If you can help make progress with this issue, please comment! T2h Time Estimate 2 Hours technical A technical issue that requires understanding of the code, infrastructure or dependencies priority-2 Second highest priority, should be worked on as soon as the Priority-1 issues are finished chore a tedious but necessary task often paying technical debt tech-debt A feature/requirement implemented in a sub-optimal way & must be re-written elixir Pull requests that update Elixir code labels Sep 7, 2023
@LuchoTurtle LuchoTurtle self-assigned this Sep 8, 2023
@LuchoTurtle
Copy link
Member

I can open a small PR over the weekend 👌

@nelsonic
Copy link
Member Author

PR: #419 merged. ✅
Thanks @LuchoTurtle 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore a tedious but necessary task often paying technical debt elixir Pull requests that update Elixir code help wanted If you can help make progress with this issue, please comment! priority-2 Second highest priority, should be worked on as soon as the Priority-1 issues are finished T2h Time Estimate 2 Hours tech-debt A feature/requirement implemented in a sub-optimal way & must be re-written technical A technical issue that requires understanding of the code, infrastructure or dependencies
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants