Skip to content

fix: Tooltips point in wrong direction#11935 #12200

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

Closed
JessieTeng89 wants to merge 5 commits into from
Closed

fix: Tooltips point in wrong direction#11935 #12200

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
68b9d17
fix: Tooltips point in wrong direction#11935
JessieTeng89 Jan 19, 2023
1b5385a
fix: linting error #11935
JessieTeng89 Jan 29, 2023
eeb33db
Merge branch 'master' into fidelity-20230119-153729
JessieTeng89 Jan 29, 2023
5ce5c8c
Merge branch 'master' into fidelity-20230119-153729
JessieTeng89 Jan 31, 2023
f68f8e0
Merge branch 'master' into fidelity-20230119-153729
JessieTeng89 Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ui/src/app/sidebar/sidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ export const Sidebar = (props: SidebarProps) => {
const locationPath = context.history.location.pathname;

const tooltipProps = {
onShow: (instance: any) => {
instance.set({
Copy link
Copy Markdown
Member

@jsoref jsoref Jan 29, 2023

Choose a reason for hiding this comment

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

Is it really not possible to do this in pure css?

Copy link
Copy Markdown
Contributor Author

@JessieTeng89 JessieTeng89 Jan 31, 2023

Choose a reason for hiding this comment

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

@jsoref I tried to investigate this again, since this tooltip is using a third-party component, all the styles are controlled by itself, but it can be covered when it is initialized just like the code change in this PR.

But also if you don't want to change any code in javascript, we have another option, we need to add a class name on the container level of the tooltip, and then add media query rules for different screen sizes in css.

Please let me know which way you preferred.

Copy link
Copy Markdown
Member

@jsoref jsoref Jan 31, 2023

Choose a reason for hiding this comment

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

In the long term, at least an issue to the 3rd party so that we could eventually fix this with css would be great.

I'm not able to approve, so I can't say whether the project is ok with your current proposal.

If this were my project and you wrote the update ticket+PR, I'd accept this change in the short term.

Copy link
Copy Markdown
Contributor Author

@JessieTeng89 JessieTeng89 Jan 31, 2023

Choose a reason for hiding this comment

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

@jsoref Thanks for your comments, either way is good for me, let me ask the owners' thoughts.

Hi @alexec @rbreeze
Could you please help on this PR? Please find more details below:
The issue the sidebar tooltip width is too large for responsive in small screens(less than 590px), I created the PR to change the tooltip width dynamically when it is initialized, but this needs change the code of javascript.

Another solution is we can add a class name on the container level of the tooltip, and then add media query rules for different screen sizes in css, in this way, we only need to change HTML and CSS.

Could you please leave your comment which way you preferred so this can be merged?

Copy link
Copy Markdown
Contributor

@saumeya saumeya Feb 21, 2023

Choose a reason for hiding this comment

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

I agree with @jsoref that this should be taken care by css. That is the media query way, there are many components which need to be responsive in the UI and it would be best to have a consistent approach for all of them.

Copy link
Copy Markdown
Contributor Author

@JessieTeng89 JessieTeng89 Nov 28, 2023

Choose a reason for hiding this comment

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

@saumeya Thanks for your suggestion, I will create another PR with media query way to fix this issue.

maxWidth: window.innerWidth - 590 > 0 ? 350 : window.innerWidth - 240
});
},
placement: 'right',
popperOptions: {
modifiers: {
Expand Down