Skip to content

[FIX]: panel header#5152

Closed
bibaswan7 wants to merge 6 commits intoopensearch-project:mainfrom
bibaswan7:fix-panel-header
Closed

[FIX]: panel header#5152
bibaswan7 wants to merge 6 commits intoopensearch-project:mainfrom
bibaswan7:fix-panel-header

Conversation

@bibaswan7
Copy link
Copy Markdown

@bibaswan7 bibaswan7 commented Sep 29, 2023

Description

When navigated to discover page and clicked on Open, the header shows "Open search", while it should be showing "OpenSearch"

Issues Resolved

#5145

Check List

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

Signed-off-by: Bibaswan Prasai <bibaswanprasai12@gmail.com>
@ashwin-pc
Copy link
Copy Markdown
Member

@bibaswan7 your PR diff looks like it contains changes that are unrelated to the issue here

@bibaswan7
Copy link
Copy Markdown
Author

@bibaswan7 your PR diff looks like it contains changes that are unrelated to the issue here

The first commit contains the necessary change to fix the issue, while the second commit is about updating the snapshot. Please let me know if I made any mistake.

@bibaswan7
Copy link
Copy Markdown
Author

I really want to make my first contribution, please help me

@ashwin-pc
Copy link
Copy Markdown
Member

Judging by the changes in your PR, it looks like the updating snapshots command is the cause of the issue. Make sure to only commit changes that you intend. I'd suggest reverting the update snapshots commit. If a snapshot needs to be updated, only update the required snapshot.

My hunch for why you see this issue is that your editor is auto formatting all these files even though it should not. But thats just a hunch since i dont know more about what the issue here is.

@bibaswan7
Copy link
Copy Markdown
Author

Oh I didn't realize my VS code was auto formatting these files. Thanks for your insight.
Anyway, I have reverted the update snapshots commit. Please let me know if its okay now.

@ashwin-pc ashwin-pc added the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Oct 3, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 3, 2023

Codecov Report

Merging #5152 (0502173) into main (677fdf5) will increase coverage by 0.03%.
Report is 3 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #5152      +/-   ##
==========================================
+ Coverage   66.72%   66.76%   +0.03%     
==========================================
  Files        3278     3278              
  Lines       62999    62999              
  Branches    10031    10031              
==========================================
+ Hits        42037    42059      +22     
+ Misses      18571    18473      -98     
- Partials     2391     2467      +76     
Flag Coverage Δ
Linux_1 35.31% <ø> (ø)
Linux_2 55.24% <ø> (?)
Linux_3 ?
Linux_4 35.42% <ø> (ø)
Windows_1 35.33% <ø> (ø)
Windows_2 55.20% <ø> (ø)
Windows_3 43.75% <ø> (ø)
Windows_4 35.42% <ø> (-0.01%) ⬇️

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

Files Coverage Δ
...plication/components/top_nav/open_search_panel.tsx 40.00% <ø> (ø)

... and 25 files with indirect coverage changes

@ananzh
Copy link
Copy Markdown
Member

ananzh commented Oct 3, 2023

I see current fix is good. Seems previous problems are all resolved. I am re-running the failed test. Meanwhile, @bibaswan7 could you add a CHANGELOG? You could find the CHANGELOG.md file and see many references.

bibaswan7 and others added 2 commits October 4, 2023 07:53
Signed-off-by: Bibaswan Prasai <bibaswanprasai12@gmail.com>
@bibaswan7
Copy link
Copy Markdown
Author

Hi @ananzh I have just added a changelog just like you asked

@kavilla
Copy link
Copy Markdown
Member

kavilla commented Oct 4, 2023

I'd suggest reverting the update snapshots commit. If a snapshot needs to be updated, only update the required snapshot.

@bibaswan7, I believe the suggestion here was to ensure the snapshot that is required to successfully run the test is updated and to avoid unrelated updates.

So to check here at the failing test: https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/6400770684/job/17378924045?pr=5152

We can see the failing snapshot coming from this test file:
src/plugins/discover/public/application/components/top_nav/open_search_panel.test.tsx

which the suggestion from Ashwin could be accomplished by running:
`yarn test:jest src/plugins/discover/public/application/components/top_nav/open_search_panel.test.tsx -u' and only pushing those changes.

Also, would you be willing to provide your local development env details?

@willie-hung
Copy link
Copy Markdown
Contributor

willie-hung commented Oct 15, 2023

Hi @bibaswan7, are you still working on this PR, since I've updated the snapshots for this specific problem and it works on my end. If you encounter any problem, I'm also glad to help. If you don't mind I can open a PR to fix this issue right now too. Thanks!

@bibaswan7
Copy link
Copy Markdown
Author

bibaswan7 commented Oct 16, 2023

Hi @bibaswan7, are you still working on this PR, since I've updated the snapshots for this specific problem and it works on my end. If you encounter any problem, I'm also glad to help. If you don't mind I can open a PR to fix this issue right now too. Thanks!

Yes, please do @Willie-The-Lord , I am really interested to see what to do with snapshots

@willie-hung
Copy link
Copy Markdown
Contributor

@bibaswan7 Based on the test you failed, which is ciGroup3, i ran the following command yarn test:jest:ci:coverage --ci-group=3 -u to update the snapshots.

@bibaswan7
Copy link
Copy Markdown
Author

@bibaswan7 Based on the test you failed, which is ciGroup3, i ran the following command yarn test:jest:ci:coverage --ci-group=3 -u to update the snapshots.

Ohh thanks @Willie-The-Lord for clarifying, I wasn't able to figure out what I was supposed to do

@joshuarrrr
Copy link
Copy Markdown
Member

I think we can close this one in favor of #5304 ?

@joshuarrrr
Copy link
Copy Markdown
Member

@bibaswan7 I'm closing as a duplicate of #5304, but please reopen if that was a mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

first-time-contributor Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry valued effort

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants