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

Fix toggle column action in the discover page #8905

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

tungsheng
Copy link

@tungsheng tungsheng commented Nov 21, 2024

Description

The DocViewerTab component needs to be updated when the columns prop is updated.

This PR

  • Add the columns prop check in the shouldComponentUpdate lifecycle method.
  • Add unit test to test if component update when columns prop changed.

Issues Resolved

fixes

Screenshot

2024-11-20 20 19 49

Testing the changes

Changelog

  • fix: Fix toggle column action in the discover page

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link
Contributor

ℹ️ Manual Changeset Creation Reminder

Please ensure manual commit for changeset file 8905.yml under folder changelogs/fragments to complete this PR.

If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link.

For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool.

Signed-off-by: Tony Lee <tony.lee@dtexsystems.com>
Signed-off-by: Tony Lee <tony.lee@dtexsystems.com>
@tungsheng
Copy link
Author

tungsheng commented Nov 21, 2024

@abbyhu2000 Would you kindly help with the WhiteSource Security Check fail in this PR? Any required action on my side? Is there a way to re-run the checks? Thank you! 🙏

@tungsheng tungsheng changed the title [WIP] toggle column button in Doc Viewer Tab [WIP] Fix toggle column action in Doc Viewer Tab Nov 21, 2024
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Signed-off-by: Tony Lee <tony.lee@dtexsystems.com>
@tungsheng tungsheng changed the title [WIP] Fix toggle column action in Doc Viewer Tab Fix toggle column action in Doc Viewer Tab Nov 22, 2024
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

@tungsheng tungsheng changed the title Fix toggle column action in Doc Viewer Tab Fix toggle column action in the discover page Nov 22, 2024
@abbyhu2000
Copy link
Member

@abbyhu2000 Would you kindly help with the WhiteSource Security Check fail in this PR? Any required action on my side? Is there a way to re-run the checks? Thank you! 🙏

let me re-run the check and see if it passes~

Copy link
Member

Choose a reason for hiding this comment

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

nice! Thanks for the unit test

abbyhu2000
abbyhu2000 previously approved these changes Nov 26, 2024
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.87%. Comparing base (073a9ff) to head (f77259b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8905   +/-   ##
=======================================
  Coverage   60.87%   60.87%           
=======================================
  Files        3802     3802           
  Lines       91072    91072           
  Branches    14375    14375           
=======================================
  Hits        55444    55444           
  Misses      32086    32086           
  Partials     3542     3542           
Flag Coverage Δ
Linux_1 29.02% <ø> (ø)
Linux_2 56.39% <ø> (ø)
Linux_3 37.89% <ø> (-0.01%) ⬇️
Linux_4 29.00% <ø> (ø)
Windows_1 29.03% <ø> (ø)
Windows_2 56.34% <ø> (ø)
Windows_3 37.90% <ø> (ø)
Windows_4 29.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 1 to 29
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Any modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the older license header, should be the following now:

/*
 * Copyright OpenSearch Contributors
 * SPDX-License-Identifier: Apache-2.0
 */

Copy link
Author

Choose a reason for hiding this comment

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

Will update it to the new license. Thank you for pointing it out!!

Signed-off-by: Tony Lee <tony.lee@dtexsystems.com>
@tungsheng
Copy link
Author

@abbyhu2000 I am not sure how to trigger the WhiteSource Security Check from my side. Could you help review and trigger the check? or let me know what I can do to trigger, thank you for your help!

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.

3 participants