-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add anchors to dashboard #1698
base: main
Are you sure you want to change the base?
Add anchors to dashboard #1698
Conversation
📦 Statoscope quick diff with main-branch: ⏱ Build time: -6.7 sec (-3.45%) ⚖️ Initial size: 0.47 kb (0.01%) 🕵️ Validation errors: 0 Full Statoscope report could be found here |
E2E Report is ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this file to src/ui/units/dash/utils
, since there are clearly dash specifics here
export const getConfiguredDashKit = (pluginDefaultsGetter: typeof currentDefaultsGetter = null) => { | ||
export const getConfiguredDashKit = ( | ||
pluginDefaultsGetter: typeof currentDefaultsGetter = null, | ||
disableHashNavigation?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use object for props here
upd: Maybe only for second argument (I saw that it breaks compatibility)
element.style.position = pos; | ||
|
||
const offsets = [ | ||
document.querySelector('.action-panel')?.clientHeight, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No one will track that the classes will change. Maybe it's better to set constants-id to elements or use data-qa?
&__wrapper { | ||
height: 100%; | ||
|
||
&:hover .dashkit-plugin-container__anchor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class doesn't work because anchor is placed outside the .dashkit-plugin-container__wrapper
I suggest putting the anchor classes in a separate file
return ( | ||
<Link | ||
onClick={(e) => { | ||
e.preventDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a link, then it should open in a new tab by cmd + click (the table of contents does so)
const newHash = props.location.hash; | ||
if (newHash !== state.hash) { | ||
newState.hash = newHash; | ||
scrollToHash({hash: newHash.replace('#', ''), withDelay: props.tabId !== tabId}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do hash.replace('#', '')
inside scrollToHash
. It seems that it is not necessary to do this here?
@@ -98,7 +86,7 @@ const TableOfContent: React.FC<{disableHashNavigation?: boolean}> = React.memo( | |||
handleToggleTableOfContent(); | |||
} | |||
if (disableHashNavigation) { | |||
scrollIntoViewWithTimeout(encodeURIComponent(itemTitle)); | |||
scrollToHash({hash: `#${encodeURIComponent(itemTitle)}`, withDelay: true}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding '#' is not necessary, is it?
No description provided.