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

FE: Broker: Config: Implement search by the Value #3804

Merged
merged 16 commits into from
Aug 23, 2023
Merged

FE: Broker: Config: Implement search by the Value #3804

merged 16 commits into from
Aug 23, 2023

Conversation

malavmevada
Copy link
Contributor

@malavmevada malavmevada commented May 10, 2023 •
edited
Loading

  • Breaking change? (if so, please describe the impact and migration path for existing application instances)

What changes did you make? (Give an overview)
In BrokerConfigTab, I have added search by value functionality and basically changed the method getData by filter out brokerconfig name or value.
Closes #3163

Is there anything you'd like reviewers to focus on?
BrokersConfigTab.java and Configs.tsx are changed. Please refer those.

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • No need to
  • Manually (please, describe, if necessary)
  • Unit checks
  • Integration checks
  • Covered by existing automation

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. ENVIRONMENT VARIABLES)
  • My changes generate no new warnings (e.g. Sonar is happy)
  • 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
  • Any dependent changes have been merged

Check out Contributing and Code of Conduct

A picture of a cute animal (not mandatory but encouraged)

@malavmevada malavmevada requested review from a team as code owners May 10, 2023 09:35
github-actions[bot]
github-actions bot reviewed May 10, 2023
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there malavmevada! 👋

Thank you and congrats 🎉 for opening your first PR on this project! ✨ 💖

We will try to review it soon!

@malavmevada malavmevada changed the title Issue/3163 Added brokerConfig search functionality by value (Issue/3163) May 10, 2023
@malavmevada malavmevada changed the title Added brokerConfig search functionality by value (Issue/3163) [fixed] Added brokerConfig search functionality by value (Issue/3163) May 10, 2023
@malavmevada malavmevada marked this pull request as draft May 10, 2023 09:57
@malavmevada malavmevada marked this pull request as ready for review May 10, 2023 09:57
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

85.7% 85.7% Coverage
0.0% 0.0% Duplication

@Haarolean Haarolean added this to the 0.8 milestone May 11, 2023
@Haarolean Haarolean added scope/frontend type/enhancement En enhancement to an already existing feature labels May 17, 2023
@Haarolean Haarolean removed this from the 0.8 milestone May 17, 2023
David-DB88
David-DB88 reviewed May 17, 2023
View reviewed changes
Copy link
Contributor

@David-DB88 David-DB88 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think the search text should be case insensitive.
keyword.toLocaleLowerCase()

@malavmevada
Copy link
Contributor Author

malavmevada commented May 17, 2023 •
edited
Loading

I think the search text should be case insensitive. keyword.toLocaleLowerCase()

The key is in lowercase and because of that i think it is converting keyword into lowerCase.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

50.0% 50.0% Coverage
0.0% 0.0% Duplication

malavmevada
malavmevada commented Jul 27, 2023
View reviewed changes
Copy link
Contributor Author

@malavmevada malavmevada left a comment

Choose a reason for hiding this comment

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

Actually, we shouldn't convert the 'item' values to lowercase because if you observe, all the values under the 'name' key are in lowercase strings. However, for the 'item' key, we can have values in uppercase, lowercase, null, and numbers. Due to this diversity, I haven't changed the 'item' values to lowercase.

kafka-ui-react-app/src/components/Brokers/Broker/Configs/Configs.tsx Outdated Show resolved Hide resolved
kafka-ui-react-app/src/components/Brokers/Broker/Configs/Configs.tsx Outdated Show resolved Hide resolved
@malavmevada malavmevada requested a review from Haarolean July 30, 2023 07:37
@Haarolean Haarolean self-assigned this Jul 31, 2023
@malavmevada
Update Configs.tsx
5207630 
Changed keyword to lowercase in getData method.
@Haarolean Haarolean self-requested a review July 31, 2023 10:33
@Haarolean
Copy link
Contributor

@malavmevada there are some linter issues which prevent the build from succeeding, otherwise looks good

@Haarolean Haarolean reopened this Jul 31, 2023
@malavmevada
Update Configs.tsx
f1a6278 
Changed getData method spaces because of some issues in linter.
@malavmevada
Copy link
Contributor Author

@Haarolean There are some issues regarding formatting the code in Linter. What should I do now ?

@Haarolean
Copy link
Contributor

@Haarolean There are some issues regarding formatting the code in Linter. What should I do now ?

I guess fixing line breaks would be nice :)
https://github.com/provectus/kafka-ui/pull/3804/files

image

@malavmevada
Update Configs.tsx
7f14ad4 
Added line breaks in getData() method to avoid linter warnings.
@malavmevada
Copy link
Contributor Author

@Haarolean Linter issue is resolved.

@Haarolean Haarolean enabled auto-merge (squash) July 31, 2023 16:59
@Haarolean
Copy link
Contributor

@malavmevada thank you, will be merged soon

@Haarolean Haarolean disabled auto-merge August 23, 2023 06:51
@Haarolean Haarolean changed the title [fixed] Added brokerConfig search functionality by value (Issue/3163) FE: Broker: Config: Implement search by the Value Aug 23, 2023
@Haarolean Haarolean merged commit ed9f91f into provectus:master Aug 23, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

42.9% 42.9% Coverage
0.0% 0.0% Duplication

gimral pushed a commit to gimral/kafka-ui that referenced this pull request Feb 23, 2024
@malavmevada @Haarolean @gimral
FE: Broker: Config: Implement search by the Value (provectus#3804)
571b99b 
Co-authored-by: Roman Zabaluev <rzabaluev@provectus.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@David-DB88 David-DB88 David-DB88 left review comments

@shailshouryya shailshouryya shailshouryya left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@Mgrdich Mgrdich Mgrdich approved these changes

@Haarolean Haarolean Haarolean approved these changes

Assignees

@Haarolean Haarolean

Labels
scope/frontend type/enhancement En enhancement to an already existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Broker: Config: Implement search by the Value
5 participants
@malavmevada @Haarolean @shailshouryya @Mgrdich @David-DB88