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

🐞🔨 Section: Deleting Sections is possible again #1900

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

zspencer
Copy link
Member

@zspencer zspencer commented Oct 19, 2023

So, this was caused by me being FANCY and putting the delete button in the same form as the edit and update. A bad idea.

Now, deleting a Section is handled in it's own independent form, rather than attempting to rewrite the forms hidden _method field to trick Rails into doing a destroy.

This is also better from a design perspective because:

A) omg, a delete button so close to the save button is DANGEROUS
B) Deleting a Section is kinda a big deal, and while it would be better to bubble-wrap the heck out of it so people who do delete a section don't wind up in a sad-mad state; we can at least give them lots of "DANGER WILL ROBINSON DANGER" messaging.

This is... irresponsible of us but what we can do now. If a motivated bystander or contributor wanted to design and implement a way for Sections to be Archived instead; I would be Very Happy ™️

- #1841

So, this was caused by me being FANCY and putting the delete button in
the same form as the edit and update. A bad idea.

Now, deleting a Section is handled in it's own independent form, rather
than attempting to rewrite the forms hidden `_method` field to trick
Rails into doing a destroy.

This is also better from a design perspective because:

A) omg, a delete button so close to the save button is DANGEROUS
B) Deleting a Section is kinda a big deal, and while it would be better
  to bubble-wrap the heck out of it so people who *do* delete a
  section don't wind up in a sad-mad state; we can at least give them
  lots of "DANGER WILL ROBINSON DANGER" messaging. This is...
  irresponsible of us but what we can do now. If a motivated bystander
  or contributor wanted to design and implement a way for Sections to be
  Archived instead; I would be Very Happy ™️
@zspencer zspencer requested review from a team October 19, 2023 00:48
Copy link
Member

@anaulin anaulin left a comment

Choose a reason for hiding this comment

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

Thank you for the 🐛🔨!

@@ -20,3 +20,17 @@
<%= render "furnitures/new", furniture: new_furniture %>
<%- end %>
<% end %>


Copy link
Member

Choose a reason for hiding this comment

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

Nit: unnecessary blank line.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wish we had an erb linter that worked

<h2>Remove this Section?</h2>
<%- data = {} %>
<%- if !room.gizmos.reload.empty? %>
<p>Warning! Deleting this Section will also delete all Gizmos and their related data. Please be very sure you want to do this! We recommend making the Section "internal" instead.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Ideally these strings would be I18n-ed, but I don't think that's a blocker for merging this.

Good call on this warning and on having a confirm on the delete button.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, ugh

# Design note: It would be far better for us to have a way to safely undo
# the deletion of a Section, or even put the Gizmos into a holding space
# or something to be re-assigned; but that is out of scope for me at the
# moment - ZS 10/18/23
Copy link
Member

Choose a reason for hiding this comment

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

Eventually: use the discard gem or similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea!

@zspencer zspencer merged commit 1c0fb1a into main Oct 19, 2023
4 checks passed
@zspencer zspencer deleted the sections/fix-bug-where-you-cant-delete-sections branch October 19, 2023 01:34
rosschapman pushed a commit that referenced this pull request Oct 23, 2023
- #1841

So, this was caused by me being FANCY and putting the delete button in
the same form as the edit and update. A bad idea.

Now, deleting a Section is handled in it's own independent form, rather
than attempting to rewrite the forms hidden `_method` field to trick
Rails into doing a destroy.

This is also better from a design perspective because:

A) omg, a delete button so close to the save button is DANGEROUS
B) Deleting a Section is kinda a big deal, and while it would be better
  to bubble-wrap the heck out of it so people who *do* delete a
  section don't wind up in a sad-mad state; we can at least give them
  lots of "DANGER WILL ROBINSON DANGER" messaging. This is...
  irresponsible of us but what we can do now. If a motivated bystander
  or contributor wanted to design and implement a way for Sections to be
  Archived instead; I would be Very Happy ™️
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.

2 participants