Skip to content

Add larger delete icon to make it easier to do on touch devices #4915

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 23, 2019
Merged

Add larger delete icon to make it easier to do on touch devices #4915

merged 3 commits into from
Jul 23, 2019

Conversation

@chrisgarrity
Copy link
Contributor

@chrisgarrity chrisgarrity commented Jun 11, 2019

Resolves

What Github issue does this resolve (please include link)?

Proposed Changes

Since it's using a new icon, don't repurpose the CloseButton and instead add a new DeleteButton with the new asset and styles.

Reason for Changes

When using the gui on ChromeOS or Android touch devices we observed that it would take multiple tries to delete a sprite.

Test Coverage

Updated the previous Delete tests to test the new component.

Browser Coverage

Check the OS/browser combinations tested (At least 2)

Mac

  • Chrome
  • Firefox
  • Safari

Windows

  • Chrome
  • Firefox
  • Edge

Chromebook

  • Chrome

iPad

  • Safari

Android Tablet

  • Chrome

@chrisgarrity chrisgarrity added this to the June 2019 milestone Jun 11, 2019
@chrisgarrity chrisgarrity requested a review from paulkaplan June 12, 2019 16:06
@chrisgarrity chrisgarrity assigned rschamp and unassigned paulkaplan Jul 16, 2019
@chrisgarrity chrisgarrity requested review from rschamp and removed request for paulkaplan July 16, 2019 12:46
rschamp
rschamp previously approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@rschamp rschamp left a comment

Choose a reason for hiding this comment

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

Looks great!

src/components/delete-button/delete-button.css Outdated
/* .delete-button.large:hover {
transform: scale(1.1, 1.1);
box-shadow: 0 0 0 4px $ui-black-transparent;
} */
Copy link
Contributor

@rschamp rschamp Jul 18, 2019

Choose a reason for hiding this comment

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

Cruft?

src/components/delete-button/delete-button.jsx Outdated
props.className
)}
role="button"
tabIndex="0"
Copy link
Contributor

@rschamp rschamp Jul 18, 2019

Choose a reason for hiding this comment

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

Eventually this should probably be a prop with a default value of 0 so it can be changed. But for now I think if they're all the same, the browser does the right thing anyway.

@rschamp rschamp assigned chrisgarrity and unassigned rschamp Jul 18, 2019
@chrisgarrity
Modify tile delete button for touch
d1e6960 
Prototype of larger delete button for sprites, costumes and sounds to better support touch devices.

I split it out from `CloseButton` as it made the changes simpler, and we may end up going in a different direction (open context menu).
@chrisgarrity
Update tests
415ecee
@chrisgarrity
Cleanup
4e47cc8 
* removed commented out cruft
* added tabIndex prop to delete button (with default=0)
* the last change also required a change to the sprite selector test snapshot
@chrisgarrity
Copy link
Contributor Author

chrisgarrity commented Jul 23, 2019

@rschamp I did the cleanup, but now there's a test failing on travis - times out converting to vector in the costumes test. However, it doesn't fail when I run it locally. Thoughts?

@rschamp
Copy link
Contributor

rschamp commented Jul 23, 2019

Let's try out the "merged into develop" version of the code locally and see if we can reproduce. And also rerun that build. It doesn't look completely unrelated to these changes even though it's not obvious why this particular action would fail.

@chrisgarrity chrisgarrity merged commit 288aa72 into scratchfoundation:develop Jul 23, 2019
@chrisgarrity chrisgarrity deleted the 4911-delete-icon branch July 24, 2019 20:12
@Secret-chest
Copy link

Secret-chest commented Apr 6, 2022

However, what does that button do? Delete the trash bin? The icon should be either a trash bin or a X, not both.

@DNin01 DNin01 mentioned this pull request Jun 20, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@rschamp rschamp rschamp left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@chrisgarrity chrisgarrity

Labels

pr - good to merge

Projects

None yet

Milestone

June 2019

Development

Successfully merging this pull request may close these issues.

Difficult to delete a sprite, costume, or sound on touch devices

4 participants

@chrisgarrity @rschamp @Secret-chest @paulkaplan