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

Delete single item #32

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Delete single item #32

wants to merge 4 commits into from

Conversation

adellava
Copy link
Contributor

@adellava adellava commented Aug 6, 2018

No description provided.

@adellava adellava self-assigned this Aug 6, 2018
@@ -9,6 +9,23 @@ const syncChartToAnchor = canvas => {
'a'
).href = `data:image/svg+xml;base64,\n${window.btoa(canvas.innerHTML)}`
})

const removeByIndexButtons = document.querySelectorAll(
Copy link
Contributor

Choose a reason for hiding this comment

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

Qui spezzerei in due la gestione. Sul componente Board gestirei il click sulle "x", la board poi emette un evento custom che index.js legge e invoca il cambio di dati. Inoltre a cosa ti serve l'attributo listner-added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mi torna, listner-added mi serve per non aggiungere N volte il listner

textNode.setAttribute('font-weight', 'bold')
textNode.setAttribute('cursor', 'default')
textNode.setAttribute('data-index', index)
textNode.setAttribute('data-remove-by-index', index)
Copy link
Contributor

Choose a reason for hiding this comment

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

Qui cambierei la gestione degli attributi. Dato che abbiamo già il data-index per indicare a quale post-it si far riferimento non aggiungerei due volte lo stesso valore. Ma dato che questo attributo ci fa da selettore possiamo semplicemente aggiungerlo ma vuoto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants