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.

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

Add destroy link in the show template #2146

Merged
merged 1 commit into from
Feb 18, 2022
Merged

Add destroy link in the show template #2146

merged 1 commit into from
Feb 18, 2022

Conversation

shouichi
Copy link
Contributor

Close #2143.

@shouichi
Copy link
Contributor Author

@nickcharlton I'm completely a noob when it comes to css. Could you help clean it up?

pablobm
pablobm reviewed Feb 17, 2022
View reviewed changes
Copy link
Collaborator

@pablobm pablobm 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 this! I left some comments that should fix the warning. After that, I think this is good to merge.

app/assets/stylesheets/administrate/components/_buttons.scss Outdated
@@ -40,6 +40,16 @@ input[type="submit"],
background-color: $action-color;
}
}

&--danger {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be moved out of the nesting, same as .button--alt and .button--nav below.

app/assets/stylesheets/administrate/components/_buttons.scss Outdated
@@ -40,6 +40,16 @@ input[type="submit"],
background-color: $action-color;
}
}

&--danger {
@extend .button;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ha. OK, the right way to solve this (if I recall correctly) is not to extend, and instead to declare both classes (button button--danger) when wanting this effect. For a similar example see:

administrate/app/views/administrate/application/_navigation.html.erb

Line 11 in b39db6b

<%= link_to(t("administrate.navigation.back_to_app"), root_url, class: "button button--alt button--nav") if defined?(root_url) %>

I know it doesn't sound very DRY, but CSS is a weird beast and this approach appears to be more manageable. Here I think we follow BEM as methodology.

app/assets/stylesheets/administrate/components/_buttons.scss Outdated
@@ -40,6 +40,16 @@ input[type="submit"],
background-color: $action-color;
}
}

&--danger {
@extend .button;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should go away when the above is fixed.

app/views/administrate/application/show.html.erb Outdated Show resolved Hide resolved
@shouichi
Copy link
Contributor Author

@pablobm Thank you for the help! The hound check is green now ✔️

@pablobm
Copy link
Collaborator

pablobm commented Feb 18, 2022

Thank you!

@pablobm pablobm merged commit e510efe into thoughtbot:main Feb 18, 2022
@shouichi shouichi deleted the destroy-link branch June 14, 2022 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablobm pablobm pablobm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding destroy link to the show template
2 participants
@shouichi @pablobm