-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Adds Main Graph Metric Selection #1364
Conversation
Next steps: - CH stuff for stat selection - Interval selection UI - Interval selection CH - Tests
…s, ToP from graphing
Thanks @Vigasaurus. Just letting you know I don't have time today to look at it and I'm off until Thu next week. So the review will take some time, sorry. Will that block your other stuff? |
@ukutaht All good - and no, not at all; I'm working on the interval selection for the graph technically on top of this in a separate branch, but without much reliance on it (it's really only to make sure I don't cause UI-based merge conflicts, since the file change overlap is almost 100%), and if nothing else, I'll have all the tests etc all sorted by the time you're back. |
Just pulled it down to look at how the UI feels before I review the code. Overall I really like it. Love how much more interactive everything is. I do have a few nitpicks on the UI:
|
I also like the #277 design. Looks very Plausible-like! Would definitely take way too much space in the top bar if its standalone. Could we somehow fit that comparison menu within date range? Search Console only shows comparison menu when you click on the date range like this: |
@metmarkosaric I like that how Search Console does it. And when you select a comparison mode, it could be shown at the top like in #277. My concern would be that I've never seen a dropdown with tabs so it might not work great in our datepicker dropdown. Maybe we should use a popup like Search Console? Gotta give it a try and see how it feels. |
Alright, should be all good to go! I also was able to add the slide up and down transition for the graph getting hidden fully as you'd mentioned + fixed up some UX on the top stat selectors. |
Amazing @Vigasaurus, thank you for sticking with it and keeping the PR up to date. Heroic effort! |
* First pass bringing in previous graph improvements, and comparsion context * Swaps issue template to new issue form syntax * Indentation update * Indentation update? * More indentation * Intendation is hard * Finalized indentation? * Github indentation * Missing fields * Formatting changes * Checkbox changes * Uses new timeseries API, various UI improvements, descopes conversions, ToP from graphing * Fixes Mobile UI Issues * Improves point detection and display on hover * Fixes & adds tests for updated main-graph API route * Changelog * Changes to better metric option declaration & minor UI/default fixes * Fixes top stat tooltips showing unformatted numbers for special (non-rounded) top stats * Formatting * Fixes regression with dashed portion not stopping at present_index * Removes comparison + lint * Improves top stat active style * Removes comparison tests * Splits out tooltip and top stats Still needs: - Tests - Potentially more cleanup * Adds/moves tests for top stats * Formatting * Updates metric LS key, removes console log * Various fixes + cleanup * Makes tooltip position & style more consistent * Fixes test (returns import status on both main graph & top stats) * Fixes interaction with month dateFormatter * Fixes edge case tooltip behavior It was simpler than I thought :/ * Make the entire top stat clickable * Minor UI improvements * Fixes another tooltip visibility edge case + cleans up boolean algebra Co-authored-by: Uku Taht <Uku.taht@gmail.com>
Some feedback:
I agree with this as well. The actual metric number is not as easy to see anymore. I see why you did it - to fit comparisons in the tooltip as well. But maybe just play around with the text styles a bit to make the metric more prominent? I can take a look as well, just posting here in case you have time to play around with it :) |
thanks @Vigasaurus! I also like your favorite one (also don't mind if you make the metric in color too). And I see no problem with making "click to view" less prominent or completely removing it. I've shared this with the person who wrote this in the first place to see what they say. |
The suggested version looks good! |
Cool - so just to validate, this version looks good for everyone? |
I get 404 on your dashboard |
I forgot to make it public - should be good now |
thanks! looks fine to me |
@jpomykala Are you sure both are actually showing as selected and it's not just showing the hover color change for one of them? (This can happen on mobile if you press and hold on the button/top stat - it gets treated as a hover). You should be able to check by just tapping anywhere else in the whitespace of the page to see if it goes back to having just the actual one highlighted. I don't particularly know of anything that could cause such an issue outside of something like that, as all the properties are fully mutually exclusive otherwise. |
Hey @Vigasaurus you are right! This probably because the hover color is the same as active color. So when I change time range, it looks like that both options are active. 😄 |
I am not sure if this will be fixed with this PR but it would be nice to go back to detailed number, so not 1.2k but for example 1256. |
@Vigasaurus this PR introduced a regression. To reproduce:
Can you take a look? |
@ukutaht Ah yeah super weird - looks like it was caused by the |
Ah sweet, thanks for letting me know! I'll commit it on master |
@metmarkosaric any chance to bring this back as well? :) |
I miss that one myself too. Any chance we can explore couple of designs that allow full numbers in the tooltip again @Vigasaurus? |
Another thing I noticed is that our mobile dashboard is no longer responsive. Not sure if it's related to this PR but it did start happening around the same time. Basically I can now scroll all the way on the right on our dashboard to an empty screen on my mobile. And when I click on any "details" views, they no longer show centrally on my screen. |
Yeah I can look into some of those, or at least just tweaking the shorting criteria for them.
That modal width issue seems not new, I see it happening even back to months back branch states (but I could be misunderstanding what you mean). (I don't use mobile enough to have seen it earlier), and I don't quite know anything in this PR that could have caused that, but it's possible. As for the actual dashboard extraneous width, I can't quite reproduce that on master - I'm happy to take a look if you have some more specific reproduction steps though. |
fyi: the mobile issue seems to have been fixed. i don't see the problems on my own phone anymore |
Changes
Resolves #117 and #532
This does also include (as discussed) the concept that if a user chooses to deselect the currently selected top stat from the graph, only the top stats show, and the graph gets hidden.
I did, however - for now - not include choosing time on page or conversion (rate/users) as an available graph selection, purely because they're not well supported (at all) in the new timeseries interface, and implementing them would not be trivial & probably shouldn't be in the scope of this PR alone. Right now, the top stat selection for showing as the graph data supports the 4 primary and default top stats (unique visitors, pageviews, bounce rate, and visit duration).
I'm also not necessarily super happy with the UI for the comparison enable/disable button in the datepicker, so I'm definitely open to ideas there.
2575756 is referring to this comical tooltip:
I'll set up some tests for this (and fix the existing tests to include the new main-graph route) soonish, but I figured it could get pushed for feedback anyways.
Tests
Changelog
Documentation
I imagine this will need both normal docs updates, and API doc updates
Screenshots