search

mlochbaum / CrinGraph

Bringing Crinacle's squiggly lines to your browser.
BSD Zero Clause License
100 stars 42 forks source link

Improvements to mobile interactions for alt layout #15

Closed MRSallee closed 3 years ago

mlochbaum commented 3 years ago

I'm not going to host Google Analytics integration in my repository. I'm not convinced GA is even a very good analytics system, and I would prefer if graph tools didn't send all the usage data to Google. I can accept the other changes if you separate them.

MRSallee commented 3 years ago

TBH I didn't realize I had any analytics in this branch. I will remove and re-PR.

My latest version of analytics implementation is more platform agnostic -- config loads a separate graphanalytics script file, and graphtool calls functions defined in that graphanalytics file. graphanalytics file then effectively functions as a plugin, where user could integrate GA or something else. Any thoughts on PRing a build with that setup, and no references to GA / no Google scripts?

mlochbaum commented 3 years ago

I'm fine with generic analytics hooks in principle. Having looked at these now, some of them seem kind of pointlessly invasive (are you planning to see if dark mode users prefer Audeze? Recording the baseline setting is also something I wouldn't expect). I guess I wouldn't reject a PR just for that though.

MRSallee commented 3 years ago

OK, recent changes remove all reference to GTM / Google, including functions. I spun up a secondary repo for me to store the GTM integration separate from CrinGraph (https://github.com/MRSallee/Graph-Anallytics-GTM).

The dark mode event was a test event to test that the event pushing is flexible enough to add new events easily. Baseline analytics I think will be interesting to see what's popular as a reference over time.

EDIT: Also I see why the analytics changes got included in this PR when I wasn't expecting -- I assumed the PR would be static based on the point in time when I made it, but now see that changes to my branch are reflecting as changes in the PR in real time. Did I mention I'm new to Github?

mlochbaum commented 3 years ago

Oh, Github pulling later changes into PRs is really annoying. You have to make the PR from its own branch if you want to keep developing.

I'm assuming setting alt_layout and analyticsEnabled to true in the config was also a mistake and have reverted those. For the color-picking dot I refactored some so it doesn't need to use currentColor. This is useful because a layout can now have both the full-size color-change button and the background highlighting for hovering on graphs.

Is the change to color_curveToText, making label text full brightness, really intentional? To me this is painful to read. Text has different rules than graphics.

MRSallee commented 3 years ago

Thanks, yes, intended to have those two settings false by default.

RE: The color for text, it is an intentional change. I find the shifted colors difficult to correlate with the colored lines. There are probably alternative executions that can solve for that + maintain high contrast for legibility, but that was the quickest way I knew to solve my issue.

I'll merge your changes into my fork and work off that for future changes. Thanks Marshall.

On Jun 1, 2021, at 1:20 PM, Marshall Lochbaum @.***> wrote:

Oh, Github pulling later changes into PRs is really annoying. You have to make the PR from its own branch if you want to keep developing.

I'm assuming setting alt_layout and analyticsEnabled to true in the config was also a mistake and have reverted those. For the color-picking dot I refactored some so it doesn't need to use currentColor. This is useful because a layout can now have both the full-size color-change button and the background highlighting for hovering on graphs.

Is the change to color_curveToText, making label text full brightness, really intentional? To me this is painful to read. Text has different rules than graphics.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mlochbaum/CrinGraph/pull/15#issuecomment-852419642, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBQGQTGPAK5I5YHQTIYYATTQU6I3ANCNFSM45YK73SA.