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

upcoming: [DI-20348] - Changes for Adding CloudPulseCustomSelect component and Integration #10807

Conversation

venkymano-akamai
Copy link
Contributor

@venkymano-akamai venkymano-akamai commented Aug 21, 2024

Description 📝

Changes for new component CloudPulseCustomSelect that helps to display various filters in the dashboard view and integration of custom select component with metrics API call.

Changes 🔄

List any change relevant to the reviewer.

  1. Created new component CloudPulseCustomSelect.
  2. Integration of the select custom filters with the metrics API call.

Target release date 🗓️

9/3/24

Preview 📷

Include a screenshot or screen recording of the change

💡 Use <video src="" /> tag when including recordings in table.

Before After
Screenshot 2024-08-21 at 12 55 32 PM Screenshot 2024-08-21 at 12 55 51 PM
Screenshot 2024-08-21 at 12 55 59 PM Screenshot 2024-08-21 at 12 56 12 PM
Screenshot 2024-08-21 at 12 56 23 PM Screenshot 2024-08-21 at 5 19 25 PM
Screenshot 2024-08-21 at 1 02 02 PM
Screenshot 2024-08-21 at 1 10 03 PM Screenshot 2024-08-21 at 1 10 35 PM

How to test 🧪

  1. Login as mock user since some of the API endpoints are not available.
  2. Go the Monitors tab.
  3. In the dashboard selection dropdown, select Dbaas Dashboard.
  4. In the Filters selection section, you can see select an Engine dropdown, that uses CloudPulseCustomSelect component.
  5. Also you can add a filter in the FilterConfig.ts file for dbaas like below and upon selection, we can see the filter goes in the metrics API request inside filters object.
{
      configuration: {
        filterKey: 'test',
        filterType: 'string',
        isFilterable: true, // isFilterable -- this determines whethere you need to pass it metrics api
        isMetricsFilter: false, // if it is false, it will go as a part of filter params, else global filter
        isMultiSelect: false,
        name: 'Test Filter',
        neededInServicePage: false,
        options: [
          {
            id: 'test',
            label: 'Test1',
          },
          {
            id: 'test2',
            label: 'Test2',
          },
        ],
        placeholder: 'Select a Test',
        priority: 5,
        type: CloudPulseSelectTypes.static,
      },
      name: 'Test Filter',
    }
  1. In the above config, if you remove the options, the error message will appear below the dropdown
    Note- You might encounter flickering issues, that is caused due to preferences, that will be solved in the upcoming PR

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@venkymano-akamai venkymano-akamai requested a review from a team as a code owner August 21, 2024 07:41
@venkymano-akamai venkymano-akamai requested review from dwiley-akamai and hkhalil-akamai and removed request for a team August 21, 2024 07:41
(CloudPulseSelectTypes.static === type &&
(!options || options.length === 0)) || (CloudPulseSelectTypes.dynamic === type && !apiV4QueryKey)
) {
staticErrorText = 'Pass pass either options or API query key';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a typo in this string?

* @param defaultSelectionProps - The props needed for getting the default selections
* @returns
*/
export const getDefaultSelectionsFromPreferencesAndPublishSelectionChanges = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we shorten the name of this function? Use the JSDoc comment instead to describe what it does.

* @param selectionChangeProps - The props needed for selecting the new filter and updating the global preferences
*/

export const callSelectionChangeAndUpdateGlobalFilters = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here about shortening this function name.

@venkymano-akamai
Copy link
Contributor Author

@hkhalil-akamai , addressed the comments, please check if it is good now

Copy link

github-actions bot commented Aug 22, 2024

Coverage Report:
Base Coverage: 82.62%
Current Coverage: 82.62%

@jaalah-akamai
Copy link
Contributor

@dwiley-akamai this is ready for review 👍

service_type: 'linode'
}),
dashboardFactory.build({
label: 'Dbaas Dashboard',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be cased as "DBaaS"?

@venkymano-akamai
Copy link
Contributor Author

venkymano-akamai commented Aug 24, 2024

@dwiley-akamai / @hkhalil-akamai - Addressed those comments including eslint issues as well, please check if it is good now

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai 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 addressing feedback & apologies for the delay in approving.

@nikhagra-akamai
Copy link
Contributor

Thanks for addressing feedback & apologies for the delay in approving.

Thank you for approval @hkhalil-akamai. @jaalah-akamai / @dwiley-akamai can we get second approval and merge if everything is fine

@venkymano-akamai
Copy link
Contributor Author

@jaalah-akamai , we have enough approvals, please check if it is good for merging

@jaalah-akamai jaalah-akamai merged commit f26a99e into linode:develop Aug 27, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants