-
Notifications
You must be signed in to change notification settings - Fork 3
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
Dashboard filters #1545
Dashboard filters #1545
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
a483490
to
792b538
Compare
792b538
to
70e8063
Compare
7d4eba7
to
6a035b1
Compare
6a035b1
to
2b44b1f
Compare
acb9441
to
6a5a667
Compare
2b44b1f
to
38369bc
Compare
38369bc
to
00f4fab
Compare
6a5a667
to
9a9595f
Compare
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.
Code LGTM, with some questions and small nits 💯
When it comes to testing the deployment preview, I think it breaks when you merge cubes:
- Go to this link.
- Add a line chart.
- Merge Heavy metal soil contamination dataset to the line chart.
- Proceed to layout options.
- Switch to Free canvas Dashboard layout.
- Enable shared filters.
- See that the filters do not work and the slider is not shown on top of the charts.
const formatSecond = timeFormat("%Y-%m-%dT%H:%M:%S"); | ||
const formatMinute = timeFormat("%Y-%m-%dT%H:%M"); | ||
const formatDay = timeFormat("%Y-%m-%d"); | ||
const formatMonth = timeFormat("%Y-%m"); | ||
const formatYear = timeFormat("%Y"); |
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.
Thinking out loud if these shouldn't be connected to RDF definitions? See that we already defined some of them here:
visualization-tool/app/rdf/parse.ts
Lines 301 to 305 in 92532b7
const timeFormatters = new Map<string, (d: Date) => string>([ | |
[ns.xsd.gYear.value, timeFormat("%Y")], | |
[ns.xsd.date.value, timeFormat("%Y-%m-%d")], | |
[ns.xsd.dateTime.value, timeFormat("%Y-%m-%dT%H:%M:%S")], | |
]); |
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.
Do you mean that we should make sure that there are the same string formats used on the client & on the server, for example having a time-formats where
export const timeFormats : Record<TimeUnit, string> = {
Year: "%Y"
Second: "%Y-%m-%dT%H:%M:%S"
...
}
and imported by both ui-helpers and rdf/parse ?
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.
Yes, I was wondering if this would make sense and is actually needed. It looks that the formatters I shared in the link are only used in interpolateTimeValues
function from the same file which is not used anywhere; so I guess we could just clean up and remove these server-side variables 👀
For the breaking of giant configurator state into smaller files, I think it's a very good step into making this more manageable, which also shouldn't really change any behavior if I am not mistaken, so I am all for it :) |
Wrap shared filter content inside ControlSectionContent for it to collapse correctly
Work in progress.
Functionality
It is now possible to activate a single shared temporal filter for all charts depending on a particular temporal dimensions.
How to test
Next steps in following PRs
Refactors
Take the opportunity to explode the big configurator-state file into multiple ones. With this, it should be easier to add changes. I'd like your feedback on this Bartosz also.