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

[Discover] Fix the overlap issue of datapicker and query editor input bar for DQL #8204

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

Conversation

RyanL1997
Copy link
Contributor

@RyanL1997 RyanL1997 commented Sep 16, 2024

Description

Fix the overlap issue of datapicker and query editor input bar

Issues Resolved

Screenshot

before:

Screen.Recording.2024-09-15.at.11.24.17.PM.mov

after:

Screen.Recording.2024-09-15.at.11.23.08.PM.mov

Testing the changes

Changelog

  • fix: Update the graph

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

❌ 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.

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.

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.14%. Comparing base (f6924a7) to head (03e5c62).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8204   +/-   ##
=======================================
  Coverage   64.14%   64.14%           
=======================================
  Files        3743     3743           
  Lines       88833    88833           
  Branches    13852    13852           
=======================================
+ Hits        56979    56980    +1     
+ Misses      31239    31238    -1     
  Partials      615      615           
Flag Coverage Δ
Linux_1 30.07% <ø> (ø)
Linux_2 58.86% <ø> (ø)
Linux_3 40.36% <ø> (?)
Linux_4 31.58% <ø> (ø)
Windows_1 30.08% <ø> (ø)
Windows_2 58.81% <ø> (ø)
Windows_3 40.36% <ø> (ø)
Windows_4 31.58% <ø> (ø)

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.

@ashwin-pc
Copy link
Member

Can we also truncate the name of the dataset if its too long? i.e. we can have it be something like testRemote::some-long-name-data be ...name-data? The Tooltip can still show the whole name

@RyanL1997
Copy link
Contributor Author

Hi @ashwin-pc, I just updated the truncate limit to 400px:

Screenshot 2024-09-16 at 4 50 34 PM
Screenshot 2024-09-16 at 4 50 49 PM

@RyanL1997
Copy link
Contributor Author

I think the tests failure are unrelated to this change and we may need a re-run for https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/10891950274/job/30223973995?pr=8204

     │1)    doc views
     │       custom doc views with datagrid table
     │         should render react doc view:
     │
     │      Error: expected 'logstash-2015.09.20' to equal 'logstash-2015.09.22'
     │       at Assertion.assert (packages\osd-expect\expect.js:111:11)
     │       at Assertion.be.Assertion.equal (packages\osd-expect\expect.js:238:8)
     │       at Assertion.be (packages\osd-expect\expect.js:80:22)
     │       at Context.<anonymous> (test\plugin_functional\test_suites\doc_views\/doc_views.ts:58:54)
     │       at processTicksAndRejections (node:internal/process/task_queues:95:5)
     │       at Object.apply (packages\osd-test\src\functional_test_runner\lib\mocha\wrap_function.js:95:16)
     │ 
     │ 
     │
     │ info [opensearch-dashboards] exited with null after 7 minutes
     │ info [opensearch] stopped
     │ info [opensearch] cleanup complete

and also found out the same BWC tests failure on the other PRs.

@ashwin-pc
Copy link
Member

The rest of the PR looks good, just these two minor comments

ashwin-pc
ashwin-pc previously approved these changes Sep 17, 2024
@ashwin-pc ashwin-pc added 2.17.1 and removed v2.18.0 labels Sep 17, 2024
&__button--truncate {
display: flex;
align-items: center;
max-width: 300px;
Copy link
Collaborator

@AMoo-Miki AMoo-Miki Sep 18, 2024

Choose a reason for hiding this comment

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

Is this a magic number? Can't it use any OUI values? Or maybe just a %?

Copy link
Contributor Author

@RyanL1997 RyanL1997 Sep 20, 2024

Choose a reason for hiding this comment

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

I just switched to a predefined EUI value. I also tried the the % but that will always leave a blank space in between the selector and the search bar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried the $euiSizeXXL, however, it looks like it is too small
Screenshot 2024-09-21 at 12 33 12 AM

also tried % and it always has a gap
Screenshot 2024-09-21 at 12 33 53 AM

iconType="arrowDown"
iconSide="right"
onClick={togglePopover}
>
<EuiIcon type={datasetIcon} className="datasetSelector__icon" />
{datasetTitle}
<span className="datasetSelector__title">{datasetTitle}</span>
Copy link
Member

Choose a reason for hiding this comment

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

could this actually use an EuiText and pass a classname className="eui-textTruncate" from OUI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just gave a try, I found that the EuiText works, however, the truncate class seems won't be applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, I also found out that after applying the EuiText the Icon happens to be at the top of the title.
Screenshot 2024-09-20 at 1 13 50 PM

@kavilla
Copy link
Member

kavilla commented Sep 19, 2024

i think for non-single line input the top row looks too large now

Screenshot 2024-09-18 at 5 24 15 PM Screenshot 2024-09-18 at 5 27 47 PM

vs the playground

Screenshot 2024-09-18 at 5 28 23 PM

@AMoo-Miki
Copy link
Collaborator

@ps48 Will this conflict with your PR?

@ps48
Copy link
Member

ps48 commented Sep 19, 2024

@ps48 Will this conflict with your PR?

Ideally shouldn't. But will rebase and resolve conflict if needed

joshuali925 added a commit to joshuali925/OpenSearch-Dashboards that referenced this pull request Sep 19, 2024
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.

5 participants