Skip to content
This repository has been archived by the owner on May 5, 2020. It is now read-only.

Clickable button for deleting a card #5

Open
5 tasks
justkd opened this issue Jul 25, 2018 · 16 comments
Open
5 tasks

Clickable button for deleting a card #5

justkd opened this issue Jul 25, 2018 · 16 comments
Labels
good first issue Good for newcomers

Comments

@justkd
Copy link
Contributor

justkd commented Jul 25, 2018

Description

Currently, a card can only be deleted by focusing it (the card itself, not a related input) and hitting your delete key.

There should be a delete button that becomes available. Perhaps it appears as an icon on the card itself when properly focused, or maybe it should be in the options container and become active when the card is focused.

Acceptance Criteria

Update [Required]

  • Create mockups for potential options
  • Add a delete button that only becomes active when a card is focused
  • The delete button should simply call the same function as using the hotkey.

Definition of Done

  • All of the required items are completed.
  • Approval by 1 mentor.
@justkd justkd added the good first issue Good for newcomers label Aug 6, 2018
@xprilion
Copy link

Hey @justkd I'd like to work on the issue.

This is the mockup I came up with - please note that the space above and below the delete button on the 'action panel' has been left to later accommodate a 'move up' and a 'move down' button.

Do let me know if the design looks good and I'll start working on it! :)
Delete button mock up

@justkd
Copy link
Contributor Author

justkd commented Jan 22, 2019

Hi @xprilion! I really like the idea of having the floating menu next to the card. Can you skip the border though? I don't think it's necessary in that position.

@xprilion
Copy link

Sure I shall skip the border (or maybe experiment with it a bit if it looks good :P )

@justkd
Copy link
Contributor Author

justkd commented Jan 22, 2019

Sounds good!

@xprilion
Copy link

Hey @justkd I have been working on the issue and realized the 'delete' button does nothing. I have tried the same on the live demo and even there it does not delete the card. Shall I add an issue for that and work on it?

@sunjunkie
Copy link
Contributor

@xprilion Do you mean the delete 'key'? It is working for me. Are you on a Mac or PC?

@justkd
Copy link
Contributor Author

justkd commented Feb 16, 2019

Hey @xprilion - make sure you have the card you are trying to delete focused. If you select a card, then select a field to edit the card, you'll have to click on the card again to delete it. Having an actual delete button would make this more intuitive as long as the icon only appears when a card is being edited!

It had to be set to only work right after clicking a card to avoid deleting characters in a text field from also potentially deleting the card itself.

@xprilion
Copy link

xprilion commented Feb 16, 2019

Hey @sunjunkie, I'm on Elementary OS 5.0 Juno (64-bit) built over Ubuntu 18.04 LTS, using the latest version of Chrome.

@justkd I tried following through the steps you mentioned, and made a small recording of what I did (on the live demo) here: https://www.useloom.com/share/628a0eb576774f08bda972489a699e1a

@xprilion
Copy link

The keycode bound to 'delete' button is 46, whereas the delete function for the card is bound to the 'backspace' key at line 1625 of app.js

const cardFocused = _ => e.which == 8 && handleDelKey()

A quick reference for keycodes: https://www.cambiaresearch.com/articles/15/javascript-char-codes-key-codes

@justkd
Copy link
Contributor Author

justkd commented Feb 16, 2019

@xprilion @sunjunkie

It just occurred to me what the problem probably is: Mac doesn't have a "backspace". The delete key on a mac is really the delete and backspace functionality from a pc rolled into one. I had tested it on windows, but I bet it's the backspace key that works, and that the delete key does return a different code, and that I didn't think about that at that time.

If all that is true, it's probably safe to add 46 as another keycode for the hotkey since I don't think it will map to anything on a Mac keyboard in that case. (But we should do a little more testing first to make sure that's really the problem.)

A quick check for the jquery e.which keycodes does say that "backspace" should be 8 and "delete" should be 46, so I think this is probably the culprit. @xprilion, please try backspace out to see if that deletes for you. I only have my macbook handy.

@xprilion
Copy link

@justkd yes 'backspace' calls the delete function on my laptop. Shall I add 46 as another keycode or we could simply update the guide to say it is 'backspace' for other systems and 'delete' for Mac.

@justkd
Copy link
Contributor Author

justkd commented Feb 17, 2019

@xprilion I think it'll be ok to just add 46 as another keycode. I went through the codes, and it doesn't appear to map to anything on mac. Thanks for pointing this out!

@xprilion
Copy link

Shall I add it with this issue or do it separately?

@sunjunkie
Copy link
Contributor

It is fine to include it in the PR for this issue. Be sure to mention it in the pull request.

@xprilion
Copy link

xprilion commented Feb 19, 2019

Hey @justkd, I created the delete button and the function to delete the card. But there's an issue - since according to the mockup I gave earlier, each card has its attached action panel. Now, on clicking any card the focus function gets called, hence clicking the button (which is a child of the card component) calls only the focus function of the card.

We might have to move out the delete button to a separate component. Shall I proceed with it? Do you have any ideas about where it can be placed, if done separately?

@justkd
Copy link
Contributor Author

justkd commented Feb 19, 2019

Instead of attaching a panel to each card, what about having a single floating panel to contain the delete button (and anything else in the future). It starts hidden, but when a card is focused, it fades in and floats next to the focused card. That way it is a separate sibling container to the card and shouldn't trigger a focus, and the delete button would only need to call the existing delete function that deletes the currently focused card. Then it fades out when a card is no longer focused, and would slide into position when a new card is focused.

Alternatively, I think it would be ok to just have a single menu panel appear in the corner of the card section instead of floating next to the card. It should also fade in/out as a card is focused. It just needs to be clear that the button only works when a card is focused.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants