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

refactor: Replace style prop with utility css classes in frontend/src/lib #23072

Closed
wants to merge 19 commits into from
Closed

refactor: Replace style prop with utility css classes in frontend/src/lib #23072

wants to merge 19 commits into from

Conversation

Dejvino
Copy link

@Dejvino Dejvino commented Jun 19, 2024

Problem

Refactoring from style prop to equivalent css classes related to issue #16826.

Changes

No visual or functional impact.

Does this work well for both Cloud and self-hosted?

Yes.

How did you test this code?

Tested through dev tools in storybook, see individual screenshots:

  • frontend/src/lib/components/...
    • InsightCard.stories.tsx
      • 01
    • CommandBar.tsx
      • 02
    • CompactList.stories.tsx
      • 03
    • RollingDateRangeFilter.tsx
      • 04
    • HedgehogBuddy.stories.tsx
      • 05
    • DebugNotice.tsx
      • 06
  • frontend/src/lib/lemon-ui/...
    • LemonCalendarSelect.stories.tsx
      • 07
    • LemonCalendarRange.stories.tsx
      • 08

daibhin
daibhin reviewed Jun 19, 2024
View reviewed changes
Copy link
Contributor

@daibhin daibhin left a comment

Choose a reason for hiding this comment

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

Left a few comments but thank you for taking this on 🙏 Let me know if there's anything that doesn't make sense - happy to work with you and get this over the line

frontend/src/lib/components/Cards/InsightCard/InsightCard.stories.tsx Outdated Show resolved Hide resolved
frontend/src/lib/components/Cards/InsightCard/InsightCard.stories.tsx Outdated Show resolved Hide resolved
frontend/src/lib/components/CompactList/CompactList.stories.tsx Outdated Show resolved Hide resolved
frontend/src/lib/components/CompactList/CompactList.stories.tsx Outdated Show resolved Hide resolved
frontend/src/lib/components/CommandBar/CommandBar.tsx Outdated Show resolved Hide resolved
frontend/src/lib/components/HedgehogBuddy/HedgehogBuddy.stories.tsx Outdated Show resolved Hide resolved
frontend/src/lib/lemon-ui/LemonCalendar/LemonCalendarSelect.stories.tsx Outdated Show resolved Hide resolved
frontend/src/lib/lemon-ui/LemonCalendarRange/LemonCalendarRange.stories.tsx Outdated Show resolved Hide resolved
daibhin
daibhin approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@daibhin daibhin left a comment

Choose a reason for hiding this comment

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

Left a few comments but thank you for taking this on 🙏 Let me know if there's anything that doesn't make sense - happy to work with you and get this over the line

daibhin
daibhin requested changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@daibhin daibhin left a comment

Choose a reason for hiding this comment

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

Left a few comments but thank you for taking this on 🙏 Let me know if there's anything that doesn't make sense - happy to work with you and get this over the line

Dejvino added 2 commits June 20, 2024 09:21
@Dejvino
Issue 16826: Replace style prop with utility css classes - frontend/s…
b27aea7 
…rc/lib

- use of standardized classes
- extracted CommandBar__overlay class
- px replaced by rem
@Dejvino
Merge remote-tracking branch 'refs/remotes/origin/master' into issue-…
144c4c7 
…16826-lib
@Dejvino
Copy link
Author

Dejvino commented Jun 20, 2024

Thank you for the feedback, @daibhin ! Updated PR with the changes.

daibhin
daibhin approved these changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@daibhin daibhin left a comment

Choose a reason for hiding this comment

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

One small suggestion but nothing blocking! Looks great - thanks for taking this on :)

frontend/src/lib/components/CommandBar/index.scss Outdated Show resolved Hide resolved
@Dejvino Dejvino changed the title Replace style prop with utility css classes (#16826) in frontend/src/lib refactor: replace style prop with utility css classes in frontend/src/lib Jun 24, 2024
@Dejvino Dejvino changed the title refactor: replace style prop with utility css classes in frontend/src/lib refactor: Replace style prop with utility css classes in frontend/src/lib Jun 24, 2024
@Dejvino
Copy link
Author

Dejvino commented Jun 25, 2024

Could I get a workflows re-run? 🙏🏻
@daibhin

@daibhin
Copy link
Contributor

daibhin commented Jun 28, 2024

@Dejvino you might need to pull the latest master and merge it into this branch for those tests to pass

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@daibhin
Copy link
Contributor

daibhin commented Jul 9, 2024

Hey @Dejvino - is this something you would like me to take over for you?

@posthog-bot posthog-bot removed the stale label Jul 10, 2024
@Dejvino
Copy link
Author

Dejvino commented Jul 10, 2024

@daibhin not yet. Sorry for the delay, holidays happened 🙂
I think I'm missing some snapshots updates to pass the tests, I'm working on that now.

@Dejvino
Copy link
Author

Dejvino commented Jul 14, 2024

Snapshots should now be updated.

FWIW this took more time than I expected due to #23699.

@Dejvino
Copy link
Author

Dejvino commented Jul 17, 2024

Hi @daibhin , I expect the last workflows failure (Error: You must pass your PostHog project's api key.) is unrelated and should be solved with the pull from master?

@Dejvino
Copy link
Author

Dejvino commented Jul 26, 2024

Ok, that didn't help. Looks like the API token is not getting passed correctly (posthog-token: ${{secrets.POSTHOG_API_TOKEN}}).

I'm afraid there is not much more I can do here @daibhin

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@posthog-bot posthog-bot closed this Aug 9, 2024
@daibhin daibhin mentioned this pull request Aug 9, 2024
Merged
@daibhin
Copy link
Contributor

daibhin commented Aug 9, 2024

Don't know what is up here @Dejvino. I just cherry picked your changes into #24293 which should hopefully work

@daibhin
Copy link
Contributor

daibhin commented Aug 19, 2024

I just merged #24293. Thank you @Dejvino for all your help on this 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daibhin daibhin daibhin approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@Dejvino @daibhin @posthog-bot