Upcoming: [DI-20844] - Tooltip for widget level filters and icons, beta feedbacks and CSS changes for CloudPulse #11062
Conversation
…onClose from Autocomplete
venkymano-akamai
requested review from
mjac0bs and
bnussman-akamai
and removed request for
a team
…value if pref is not available
upcoming: [DI_21359] - Added auto interval option as default interval value if pref is not available
| @@ -140,6 +140,7 @@ export const CloudPulseWidget = (props: CloudPulseWidgetProperties) => { | |||
| widget: widgetProp, | |||
| } = props; | |||
| const flags = useFlags(); | |||
| const scaledWidgetUnit = React.useRef(unit === 'Bytes' ? 'B' : unit); | |||
There was a problem hiding this comment.
| const scaledWidgetUnit = React.useRef(unit === 'Bytes' ? 'B' : unit); | |
| const scaledWidgetUnit = React.useRef(generateCurrentUnit(unit)); |
|
This is looking better - just waiting on the tooltip update before approval |
@jaalah-akamai , we have moved to the common tooltip |
…ure/beta_feedbacks_plus_bugs
| }} | ||
| autoHighlight | ||
| disableClearable | ||
| fullWidth={false} |
There was a problem hiding this comment.
I believe fullWidth defaults to false. It is probably unnecessary to explicitly specify it.
| fullWidth={false} |
Please try to avoid adding props unless they are necessary.
| label="Select an Aggregate Function" | ||
| noMarginTop={true} | ||
| options={availableAggregateFunc} | ||
| sx={getAutocompleteWidgetStyles(theme)} |
There was a problem hiding this comment.
You can actually pass a function to sx. The theme will be passed automatically, so you can even remove the useTheme hook above
| sx={getAutocompleteWidgetStyles(theme)} | |
| sx={getAutocompleteWidgetStyles} |
| }} | ||
| autoHighlight | ||
| disableClearable | ||
| fullWidth={false} |
There was a problem hiding this comment.
Same feedback here. Is this nessesaery?
| _: React.SyntheticEvent, | ||
| selectedInterval: IntervalOptions |
There was a problem hiding this comment.
Now that the type-safety issue is fixed, please remove these explicit types. Allowing typescript to infer the types will be more type-safe
| option: IntervalOptions, | ||
| value: IntervalOptions |
There was a problem hiding this comment.
Same thing here. No need to explicitly put the types now that the type-safety is fixed
| label="Select an Interval" | ||
| noMarginTop={true} | ||
| options={[autoIntervalOption, ...availableIntervalOptions]} | ||
| sx={getAutocompleteWidgetStyles(theme)} |
There was a problem hiding this comment.
Same thing here. sx can take a function. This will allow us to remove the useTheme call above
| sx={getAutocompleteWidgetStyles(theme)} | |
| sx={getAutocompleteWidgetStyles} |
There was a problem hiding this comment.
Should we keep this abstraction or just use Tooltip directly? cc @jaalah-akamai This seems fine, but I feel like one could argue is it an extra abstraction that would be nice to not need if possible
There was a problem hiding this comment.
@bnussman-akamai , thanks for the suggestion, I suggest we can keep this abstraction, so in future any CloudPulse specific changes can go in one tooltip component rather than doing it in all filters.
|
@bnussman-akamai , I have addressed the comments you have left. Please check once |
jaalah-akamai
added
Approved
|
I had to re-run the CI E2E... when it passes, we'll merge |
…ure/beta_feedbacks_plus_bugs
|
@jaalah-akamai , for this PR, in the same branch, in local if run yarn coverage:summary, yarn test, our ACLP cy, everything is passing. I don't know whats with the CI |
Description 📝
We have added Tooltip for all of our icons and widget level filters. Also addressed some of the beta demo feedbacks like
CamelCase for Aggregation Function dropdown label, Zeroth state of time duration and CSS updates for legend rows inside the widget.
Changes 🔄
Target release date 🗓️
10-10-2024
Preview 📷
How to test 🧪
As an Author I have considered 🤔
Check all that apply