-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
refactor: Small fixes to product analytics goals + layout #28754
refactor: Small fixes to product analytics goals + layout #28754
Conversation
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.
PR Summary
This PR focuses on synchronizing frontend-only properties between frontend and backend code, particularly for insight visualizations. Here's a summary of the key changes:
- Added consistent handling of "Add filter group" buttons in
InsightViz.scss
with conditional display based on layout mode - Synchronized frontend-only property filtering between
queryUtils.ts
andschema_helpers.py
to maintain consistency - Added support for goal lines in trends insights by including
goalLines
in visualization-only fields - Updated display type handling with canonical values for chart categories
- Added explicit cross-references between frontend and backend code for property filtering
The changes appear well-structured and improve the maintainability of the codebase by keeping frontend-only properties consistently handled across the stack.
5 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
📸 UI snapshots have been updated32 snapshot changes in total. 0 added, 32 modified, 0 deleted:
Triggered by this commit. |
Size Change: 0 B Total Size: 1.21 MB ℹ️ View Unchanged
|
Changes look good, but it seems to have broken cypress tests @rafaeelaudibert |
Yeah, fixing that now, had no power at home this morning. I just need to keep one of the buttons with the previous ID. Will fix soon, thanks for the stamp! |
These have gotten out of sync because it was hard to find them in the backend, let's put them back in sync and also update the
Display can be empty and in that case we display a `ChartDisplayType.ActionsLineGraph`, so let's detect that
Rather than displaying button inline at all times, let's display it inline on horizontal mode, and then under the filters on vertical mode
If display is not present, just treat as if it was an `ActionsLineGraph` - like the rest of the code does
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
This is breaking Cypress
c25f5ce
to
0093bd4
Compare
Follow-up to #28461