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

Chore: Rewrite alerting css using object styles #87114

Merged
merged 4 commits into from May 7, 2024
Merged

Conversation

kaydelaney
Copy link
Contributor

@kaydelaney kaydelaney commented Apr 30, 2024

If any reviewers would prefer for this PR to be broken up into several smaller ones let me know!

Edit: Some details for context

The process for converting the style formats was as follows:

  1. Ran a grep for any ts/tsx/js/jsx files with a match for "css`"
  2. Wrote a very hacky deno script to do some text processing on the matched files
  3. Ran yarn start, checking for any failures or type issues (sometimes this surfaced issues with the original css, which I then fixed)
  4. Ran prettier on any changed files to fix up the formatting
  5. Went through each file the script couldn't handle. Typically the files in question were ones with multi-line conditional logic or ones that used css selectors.
  6. Manually fixed the styles for any classes that were causing problems, and then re-ran the script to catch the simple cases.
  7. Reviewed each of the changed files one by one to make sure there weren't any mistakes.

For completeness, here's the script I wrote: https://gist.github.com/kaydelaney/489bac318fe99febd7d0cf3e40b2071b
(Don't judge it too harshly 🙏 )

@kaydelaney kaydelaney added area/alerting Grafana Alerting type/chore area/frontend type/debt technical debt no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Apr 30, 2024
@kaydelaney kaydelaney self-assigned this Apr 30, 2024
@kaydelaney kaydelaney requested review from grafanabot and a team as code owners April 30, 2024 09:11
@kaydelaney kaydelaney requested review from gillesdemey, tomratcliffe, konrad147 and soniaAguilarPeiron and removed request for a team April 30, 2024 09:11
@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.1.x milestone Apr 30, 2024
@gillesdemey
Copy link
Member

Fixed the merge conflicts from #84501 :)

Copy link
Member

@gillesdemey gillesdemey left a comment

Choose a reason for hiding this comment

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

Thanks for putting in all of the work @kaydelaney!

Haven't reviewed all the changes but we've agreed to just go ahead and stamp this for approval – the Alerting team will keep on eye on any weird or odd style changes and we'll just fix them as they come.

I've taken the PR for a quick spin and no major functionality seems impacted :)

LGTM! :shipit:

@kaydelaney kaydelaney merged commit bc67b88 into main May 7, 2024
14 checks passed
@kaydelaney kaydelaney deleted the alerting-css branch May 7, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Grafana Alerting area/frontend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes type/chore type/debt technical debt
Projects
Status: Done
Status: 🚀 Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants