-
Notifications
You must be signed in to change notification settings - Fork 78
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
fix(ValueCard) Remove XSMALL from LEGACY_CARD_SIZES because we are using and displaying unwanted warnings #3935
base: next
Are you sure you want to change the base?
Conversation
…ing unwanted warnings
✅ Deploy Preview for carbon-addons-iot-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -9,6 +9,7 @@ export const COLORS = { | |||
}; | |||
|
|||
export const CARD_SIZES = { | |||
XSMALL: 'XSMALL', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this supported ? As far as I'm aware support for XSMALL was removed. and when ever someone use XSMALL we used to change it to 'SMALL'
This may have impact on monitor dashboards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@herleraja Hello. Support for xsmall was removed, but we have a scenario in Maximo where the use of it is required again. It will only be allowed for certain simple Value cards, and there is likely some CSS cleanup and other work that will need to go in to that card type. What would work for Monitor here? Is there a condition or check we could add to isolate you from this change? @anishkumar-bhut @hectordavis @stuckless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where exactly is the warning coming from? From what I can see here, ValueCard still uses the LEGACY_CARD_SIZES as expected sizes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JordanWSmith15 Just checked. I think the monitor should not be affected by this change.
We override the props accordingly.
Also, this may need to be well tested, because its used directly or indirectly
example if the size is small we won't display the error message icons. so we need to add the XSMALL size here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hectordavis the warning thrown in getUpdatedCardSize but that's thrown only in development mode. It should not effect in production.
@@ -92,9 +92,7 @@ export const compareGrains = (grain1, grain2) => { | |||
|
|||
export const getUpdatedCardSize = (oldSize) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
in ValueCard component have a CARD_SIZES and in that XSMALL is under LEGACY_CARD_SIZES but in Operation dashboard and other places XSMALL is using so in console unwanted warning displaying so added XSMALL in CARD_SIZES
Change List (commits, features, bugs, etc)
Acceptance Test (how to verify the PR)
Regression Test (how to make sure this PR doesn't break old functionality)
Things to look for during review
iot
orbx
class prefix is using the prefix variabledata-testid
attribute. New test ids should have test written to ensure they are not changed or removed.