Conversation
Top bar - Add consistent styling and spacing for all buttons in the top bar - Move period arrows into the period picker visually instead of displaying them as separate buttons - Add selected state to buttons; when dropdown is open, the trigger background is light gray. This is also true when comparison is enabled. - Change filter button icon from magnifying glass to a filter icon - Change period picker button icons from chevron on the right to calendar icons on the left - Make top bar scroll horizontally on mobile instead of clipping New options menu (⋮) - Move interval picker, export button, imported data toggle and notices out of the graph and into a new options menu in the top bar - Hide menu in realtime mode - Render button immediately as a non-interactive placeholder on page load to prevent layout shift - Change interval picker dropdown to a segmented control - Change the imported data toggle to an actual toggle button - Change notice icon + tooltip to an inline notice component Top stats - Change selected metric background highlight to a subtle background instead of a coloured underline on the label - Refine stat labels and values typography and colors - Ensure change arrows and percentages are shown also when in comparison mode (and removed from tooltip) - Show "until HH:MM" when "Today" is the comparison date range to indicate a partial day Code organisation - Introduce a shared React context (DashboardOptionsProvider) to pass graph state (intervals, import settings) up to the top bar without prop drilling - Move custom icons to a shared icons file - Extract segmented control, toggle button and notice as reusable components - Old standalone StatsExport, WithImportedSwitch, and NoticesIcon components removed; functionality consolidated into the options menu
d0eeb10 to
269eac4
Compare
07c0dc1 to
f334e4d
Compare
|
|
Tested on preview. Feels great to me overall. A couple things I noticed while testing: 1. Date picker has uneven padding when arrows not shown
The padding looks good with arrows. 2. Date picker dropdown has some weirdness I cannot close the dropdown by clicking outside of it just above the datepicker: https://cap.so/s/8dg13mgn12zk6bb Also I cannot replicate this for some reason but once I did manage to get it into this state where the focus ring shows and does not look great
|
- Refactor site switcher to use a static component when not logged in - Add test for static site switcher - Fix padding on period picker when arrows are hidden - Fix focus-visible ring on period picker - Update typography for site switcher and current visitors
ac3223d to
7575f95
Compare
|
@ukutaht good catches, fixed them! RE typography of the site switcher and current visitors: it was a choice between consistency and hierarchy, and I opted for more consistency. I was hesitating as well though so I reverted it to the original. I think because the site domain isn't capitalised and the current visitors starts with a number, the latter feels heavier somehow. |
| height: SEE_MORE_WIDTH_PX, | ||
| width: SEE_MORE_WIDTH_PX, |
There was a problem hiding this comment.
nitpick, non-blocking: The reason size was given with style is for the actual size and the size used in calculations not to be out of sync. Now, the sync between size-8 and SEE_MORE_WIDTH_PX has to be managed manually.
| {showCurrentVisitors && <CurrentVisitors />} | ||
| </div> | ||
| <div className="flex w-full"> | ||
| <div className="flex min-w-0 flex-1"> |
| }: { | ||
| children: React.ReactNode | ||
| }) { | ||
| const [options, setOptions] = useState<DashboardOptionsContextValue | null>( |
There was a problem hiding this comment.
issue, non-blocking: This is duplicative state (between visitor-graph and this component), which causes us to need syncing code. I think a better split would be to have
GraphIntervalContext - solely responsible for calculating available intervals and the current one, and providing the means to change the current one
ImportsIncludedContext - same as above, just for imports, may itself depend on GraphIntervalContext
and to use the exposed context state in the components where they're needed, when fetching the graph and in the new triple dot menu.
| ) | ||
| }) | ||
|
|
||
| describe(`${getPartialDayTimeRange.name}`, () => { |
There was a problem hiding this comment.
suggestion, non-blocking: Let's also test that it handles timezones in the date string (I've not actually seen 2024-01-13T00:00:00, always something like
"date_range": [
"2026-01-19T00:00:00+00:00",
"2026-04-19T23:59:59+00:00"
],
| return { topStats, meta, from, to, comparingFrom, comparingTo, timeRange } | ||
| } | ||
|
|
||
| // Returns "until HH:MM" when the date range is a partial day (period=day for |
There was a problem hiding this comment.
suggestion, non-blocking: If the comment about the function is exactly above the function, IDEs include it when hovering at a call site.
/** Does something complex, explanation */
const f = () => {
// ..
}
| {formatDateRange(site, data.comparingFrom, data.comparingTo)} | ||
| <p className="text-xs text-gray-500 dark:text-gray-400 font-medium"> | ||
| {data.timeRange | ||
| ? `${formatDayShort(parseUTCDate(data.comparingFrom))}, ${data.timeRange}` |
There was a problem hiding this comment.
issue, blocking: This works for Today vs Previous period, but it is incorrect when comparing Today vs Custom Period (pick yesterday from comparison calendar). The comparison query is made for the whole day, so we'd need to calculate and show comparisonTimeRange here.
|
|
||
| expect(arrowElement).toHaveTextContent('↑ 1%') | ||
| expect(arrowElement.children[0]).toHaveClass('text-green-500') | ||
| expect(arrowElement.children[0]).toHaveAttribute('data-direction', 'up') |
There was a problem hiding this comment.
question, non-blocking: Why did we get rid of the color asserts? I think they matter because bounce rate down being rendered as green is important. Up/down direction is already tested with toHaveTextContent('↑ 1%'). (The arrow icons are mocked with a particular unicode character)
| className={classNames( | ||
| popover.toggleButton.classNames.rounded, | ||
| 'gap-x-1.5 px-2.5 font-medium text-gray-700 dark:text-gray-100', | ||
| '!pl-0' |
There was a problem hiding this comment.
question, non-blocking: why padding left with !important? I don't see padding left previously declared in popover.toggleButton.classNames.rounded, just on the prev line.
| 'hover:bg-gray-100 dark:hover:bg-gray-800' | ||
| popover.toggleButton.classNames.rounded, | ||
| popover.toggleButton.classNames.ghost, | ||
| '!pl-1.5' |
There was a problem hiding this comment.
question, non-blocking: Does the switcher not need same left & right padding? E.g. !px-1.5
| @@ -627,10 +640,7 @@ test.describe('location filtering tests', () => { | |||
|
|
|||
| await applyFilterButton(page).click() | |||
|
|
|||
| await page | |||
| .getByRole('button', { name: 'See 1 more filter and actions' }) | |||
| .click() | |||
|
|
|||
| await page.getByRole('button', { name: /See.*more/ }).click() | |||
There was a problem hiding this comment.
issue, non-blocking: These tests are running on a constant viewport (1024px). "See N more" depends on that, filter pills style, and other top bar items.
Because the UI changed, the conditions to wrap to to "See N more" changed, so I can see why you added another filter. However, the comment is misleading and there's some missing asserts now.
In these tests, I think we should definitively check which filters of the added filters are visible without clicking "See more", that "See more" has the correct label, and that all the rest are visible after clicking "See more".
|
Great work! Pretty major changes at first glance, but opening the app it feels very familiar and natural, at least to me. With the top stats enhanced time labels issue solved (#6245 (comment)), it's ready to go as far as I'm concerned. |





Changes
Top bar
New options menu (⋮)
ikeyboard shortcut now cycles through graph intervals rather than opening the dropdown menu – also when the options menu is closedTop stats
Code organisation
Before and after:
https://github.com/user-attachments/assets/ec519314-629f-4034-87d9-9cded458f646
New options menu:

Tests
Changelog
Documentation
Dark mode