-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Improve reports layout #5962
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
base: master
Are you sure you want to change the base?
Improve reports layout #5962
Conversation
- Create new `ReportLayout` and `ReportHeader` components to ensure consistent layout across reports - Remove title from report header, and place tabs in the top left corner - Move the details button to the top right corner, rather than the bottom of the report, to reduce vertical space and improve readability. Reduce it to a simple icon with a 'View details' tooltip. - Update the Search terms report to use the new `ReportLayout` and `ReportHeader` components. - Update dashboard to use a simple grid layout, simplifying the html markup and css. - Change 'Screen sizes' title to 'Devices' in the devices report. - Change 'Top pages' title to 'Conversion pages' in the pages report, whenever a conversion goal filter is applied. - Add 'last 30min' pill to the behaviour report when on realtime dashboard, rather than displaying this in the title. - Remove the Combobox input from the custom properties report, and use the same tab dropdown pattern as used for funnels and UTM campaigns.
|
…tes when special goal filter is applied
… goal filter and realtime are active
898b1f4 to
b430b6c
Compare
zoldar
left a comment
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.
LGTM, though asking @RobertJoonas to have a second look at any React-related peculiarities I might have missed 🙏
| import { useCallback, useState } from 'react' | ||
| import { AppNavigationLinkProps } from '../navigation/use-app-navigate' | ||
|
|
||
| export function useMoreLinkData() { |
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.
issue, blocking: this hook duplicates state (creates redundant state), which is probably the reason we need
// eslint-disable-next-line react-hooks/exhaustive-deps where it's used.
To fix, we can get whether the list is loading and list data from hook props.
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.
Thanks for spotting this 🙏 Fixed as per @RobertJoonas's suggestion.
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.
Thanks!
It definitely works, although it still duplicates list data and loading state. The reason duplicated state is not good is because then there's a risk that the original source state and the duplicated state get out of sync. Out of sync state can cause UI glitches, sometimes pretty severe ones. However, it's not a blocker when taking into consideration the fact that we're getting rid of React soonish.
I think I can see why you needed to do it: MoreLink is higher in the component tree than the component that holds state with query results and query loading status. afterFetchData is also a workaround for that fact.
Just to share my view on this, not requesting you make any changes, I think the most appropriate solution would have been to lift the logic with query results and loading state logic higher up (to the level that renders MoreLink or above) and pass that state down, rather than to bubble it up from below.
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.
Yeah I see what you mean, that would be better indeed. I'll leave it up to you whether you think we should invest the time? (I think I'd need help with that if we wanted to go that route.)
|
When I filter by goal, the percentage column becomes static, is that intended? If so, what's the reasoning there? Why do they have to slide in on hover in the unfiltered view? If I use the mouse cursor to just point where I'm looking at, it feels distracting and makes me wonder what can I do to keep the calm experience from "filter by goal" view. |
RobertJoonas
left a comment
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.
Looks good to me in general! I'm delighted to see the prop key picker finally looking decent 👏 Left some comments inline. I think there's a way to simplify the MoreLink rendering logic significantly.
Also, we could probably move the CTA notice and "no data" text more in the middle in the custom props report. WDYT?
|
|
||
| useEffect(() => { | ||
| if (searchable && showSearch && searchRef.current) { | ||
| const timeoutId = setTimeout(() => { |
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.
Q: Why are we focusing the search in 100ms and not instantly?
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.
The delay is to account for the animation of the popover.
| path: browsersRoute.path, | ||
| search: (search) => search | ||
| }} | ||
| onListUpdate={onListUpdate} |
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.
I think we can simplify things up a bit.
- Here (and in other call sites of ListReport) we can avoid introducing
onListUpdateas a new prop and just include its behaviour inafterFetchData - Avoid the new
useMoreLinkDatahook
The component structure looks something like this (example of Devices, but it's pretty much the same everywhere else):
- Devices
- MoreLink
- Browsers
- ListReport
- OperatingSystems
- ...
The MoreLink component doesn't actually need the list of data in order to render itself. It just needs to know:
- whether or not to render at all
- whether or not it should actually be a clickable link
- the linkProps to construct an
AppNavigationLink
The first two can be achieved via the afterFetchData hook. For the third one, we need to move detailsLinkProps construction from ListReport props into Devices (with the value depending on the currently selected tab).
Here is more or less how I imagine it could work:
// in devices/index.js
const [moreLinkVisible, setMoreLinkVisible] = useState(true)
const [moreLinkClickable, setMoreLinkClickable] = useState(false)
// the function already exists there
function afterFetchData(apiResponse) {
setLoading(false)
setSkipImportedReason(apiResponse.skip_imported_reason)
// this needs to be added
if (response.results.length > 0) {
setMoreLinkClickable(true)
} else {
setMoreLinkVisible(false)
}
}
function moreLinkProps() {
switch (mode) {
case 'browser':
return {
path: browsersRoute.path,
search: (search) => search
}
case 'os':
return {
path: operatingSystemsRoute.path,
search: (search) => search
}
// etc...
}
}
return (
// ...
<MoreLink
visible={moreLinkVisible}
clickable={moreLinkClickable}
linkProps={moreLinkProps()}
>
)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.
Thank you so much for the detailed feedback! 🙏 I've updated this, and also changed the visible and clickable states to an enum for better state management.
Also centered the CTA – good catch!
@aerosol this is part of a different PR/feature, you can find the full discussion here. Feel free to add your feedback as we've been going back and forth on this decision. The reason for the percentage not being permanently visible is to keep the dashboard default clean, and percentage is only a "supporting" metric. When filtered by goal, the |
assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx
Outdated
Show resolved
Hide resolved
| DETAILS | ||
| </AppNavigationLink> | ||
| </div> | ||
| <Tooltip info="View details" containerRef={portalRef}> |
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.
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.
I fixed the issue by updating the z-index – do you think we should prevent the tooltip from showing altogether when an element is clickable on mobile? I'm not sure if that's overengineering it and possibly causing undesired knock-on effects 🤔
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.
…ion.tsx Co-authored-by: Artur Pata <[email protected]>

Changes
ReportLayoutandReportHeadercomponents to ensure consistent layout across reportsReportLayoutandReportHeadercomponents.5to10in the tab dropdown.Ultra-wideand(not set)device sizes.Tests
Documentation
Dark mode