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

Lf 4747 create sensor readings kpi component #3705

Open
wants to merge 33 commits into
base: integration
Choose a base branch
from

Conversation

Duncan-Brain
Copy link
Collaborator

@Duncan-Brain Duncan-Brain commented Mar 4, 2025

Description

Waiting on final approval from Loic for multiple parameters styling see link in Jira comments

  • Tiles: This creates KPI tiles for use with sensors arrays and individual sensor metrics.

  • Icon: Adds the ruler Icon.

  • Layout: It also creates a reusable layout for composition of tiles.

    • It copies the currently named "bento" style of sensor list overview stats -- let me know if there is a different preferred name.
    • It is technically a drop in replacement for the top level div of overview stats

Notes

  • Storybook: I had some issues with storybook loading css correctly -- if you have issues a refresh or restart should do the trick.

Jira link: LF-4747

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain)

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have added "MISSING" for all new language tags to languages I don't speak
  • I have added the GNU General Public License to all new files

Sorry, something went wrong.

@Duncan-Brain Duncan-Brain marked this pull request as ready for review March 4, 2025 23:44
@Duncan-Brain Duncan-Brain requested review from a team as code owners March 4, 2025 23:44
@Duncan-Brain Duncan-Brain requested review from SayakaOno and kathyavini and removed request for a team March 4, 2025 23:45
Copy link
Collaborator

@SayakaOno SayakaOno left a comment

Choose a reason for hiding this comment

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

I left a few comments, but it looks great! Thank you!

SayakaOno
SayakaOno previously approved these changes Mar 6, 2025
@kathyavini kathyavini requested review from antsgar and removed request for antsgar March 7, 2025 23:47
Copy link
Collaborator

@kathyavini kathyavini left a comment

Choose a reason for hiding this comment

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

Very interesting PR -- a ton of creative CSS solutions here! <BentoLayout> as a wrapping component reminds me of a proper component library, very neat ☺️

@SayakaOno since you've already approved and are the one using this component in your views, please go head and merge if it's blocking you 🙏

The only code that really needs to be fixed I think is the RegEx since it doesn't work (if that 3 to 6 digit hex functionality is something the charts need) but even that I don't think is necessarily blocking.


.icon {
padding: 0px;
background-color: transparent;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it just my Storybook this doesn't work for? I saw a screenshot in the Figma thread where it was fine 😂 But for me I need to over-specify this class.

Screenshot 2025-03-10 at 9 29 49 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kathyavini Did you try restarting Storybook? I had the same issue -- and I have it all the time where classes where I think I should override look funny sometimes but not others it is so flakey! Even for the classes I did over-specify in this PR I could not for the life of me figure out why it was necessary, I literally have never had to over specify before.

I think there is an issue with vite not being able to compile properly on hot reloads. So by extension storybook-vite I think has the same issue.

@Duncan-Brain
Copy link
Collaborator Author

Thanks for reviewing! I did try to make it like MUI -- I am now trying to understand when best to use "composition" (children prop) and make better components.

I updated the component to take validation of color out of it -- that can go wherever color is decided.

I updated stories since I did not realize the ability to edit parameters was negatively affected in the previous story and not possible. This is a better story but my typescript is a bit off. Maybe some future me can fix it but it is better to have a useful story than perfect types.

@Duncan-Brain Duncan-Brain requested a review from antsgar March 18, 2025 13:57
kathyavini
kathyavini previously approved these changes Mar 19, 2025
Copy link
Collaborator

@kathyavini kathyavini left a comment

Choose a reason for hiding this comment

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

Looks good!! 😍

but it is better to have a useful story than perfect types.

💯 for sure! Your typescript is only a smidge off anyway. Left a few optional comments for now or later if you do want to remove the ts-expect-errors 🙏

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.

None yet

4 participants