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

Display cron schedules args in dashboard #396

Merged
merged 1 commit into from Oct 3, 2021

Conversation

aried3r
Copy link
Contributor

@aried3r aried3r commented Oct 2, 2021

As promised the follow-up. Now there's quite a bit of code duplication. Do you want me to deduplicate the code or do we go by the rule of three?

Removed the # from aria-controls as the list of ID references doesn't use the # afaik.

Added args to the cleanup job so that we cover all cases for this column again.

Video shows that the buttons still trigger what they're supposed to.

Kapture.2021-10-02.at.22.30.25.mp4

Copy link
Owner

@bensheldon bensheldon left a comment

Choose a reason for hiding this comment

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

I think this is strong as-is 👍

Personally, I find that extracting/abstracting reusable html partials/helper logic can get gnarly quickly, and then become an obstacle to contributors who want to suggest UI improvements at the HTML level, but have to drop into some twisty Ruby code to do so.

I'd like to keep the barrier to contributing to the Dashboard low at least for now while I'm still learning about how people want to use it.

@bensheldon bensheldon merged commit 07335d6 into bensheldon:main Oct 3, 2021
@bensheldon bensheldon added the enhancement New feature or request label Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants