Closed ud3sh closed 1 year ago
Latest commit: a9fb4e55e0672f3a1e07e2db5e9baff7ca227398
The changes in this PR will be included in the next version bump.
Not sure what this means? Click here to learn what changesets are.
Click here if you're a maintainer who wants to add another changeset to this PR
The latest updates on your projects. Learn more about Vercel for Git ↗︎
Name | Status | Preview | Updated |
---|---|---|---|
evidence-docs | ✅ Ready (Inspect) | Visit Preview | Jun 23, 2022 at 1:33PM (UTC) |
This is EPIC. Overall love it
Should we put somewhere:
Evidences support Excel style formatting codes.
Nit picky thoughts:
Thanks @archiewood for the feedback. My answers to your Qs.
Do we need the example input for the Built ins?
This was part of the original spec. I find it useful for myself, but not sure if it helps the user and does add some noice as is like you said. I think the idea was long term we'll let users change the default formats. I am open either-way. //cc @hughess
I think the output example for the id column is unhelpful? Isn't the point that it removes commas, so a more helpful example
Yeah, I think I messed up the example for ID. Will fix it.
numk / numm / numb are slightly confusing format tags. Assume num-k is not allowed?
Currently, I only allowed letters the format name. The main reason for this is that as formatName
aliasing needs to work with all DBs to actually apply the custom format. I think '-' won't work with most DBs, although numbers and _ should work with all DBs as long as they are not the leading character. I disallowed those as well just to keep things simple for now but we could certainly reconsider it.
Happy to hear other views on this, but I would expect a small k to be formatted (as in kmph). Possibly also a small m for million. B I think makes sense to be big. $4k , $4m, $4B
I decided to go with k, M, B since it was more prominent in examples. I am open to either - if there is a standard or something we can refer to, it would be ideal.
)
This is EPIC. Overall love it
Agreed 1,000%, this looks awesome.
Also, I scoped creeped basic code formatting with prettier.
lol nice.
Re: process here -- Should we start tracking an issue for moving this to GA (e.g. out from behind the feature flag)?
I have a couple questions on the settings UX, but I don't think they need to be resolved in this release, given it'll be feature flagged.
This is fantastic! 🎉
Example Inputs
On the example inputs, I think they're helpful to play around with and see what the formatted result would be. I agree about adding noise to the page though. Maybe we could make that column narrower by changing all the date example inputs to YYYY-MM-DD
(+ time if applicable).
Format Names
Currently, I only allowed letters the format name. The main reason for this is that as formatName aliasing needs to work with all DBs to actually apply the custom format. I think '-' won't work with most DBs, although numbers and _ should work with all DBs as long as they are not the leading character. I disallowed those as well just to keep things simple for now but we could certainly reconsider it.
I think we should allow numbers to be a part of the custom format tags (e.g., if someone wanted to create variations of a custom format with different decimal places). I think leaving out underscores makes sense for now.
Units On units, I'd also expect to see a small "k", but I like the uppercase versions for millions and billions.
Including strings in format codes Is it possible to use quotes instead of escaping characters for strings inside the format codes? Example for CHF below:
That would make the formats consistent with the other formats where characters are included, like "M" and "B".
Re: process here -- Should we start tracking an issue for moving this to GA (e.g. out from behind the feature flag)?
Yeah, I think we'd want an issue in the next cycle to clean this up based on feedback. I'll consolidate the suggestions that were listed here (besides the obvious bugs which I'll fix before the merge).
I have a couple questions on the settings UX, but I don't think they need to be resolved in this release, given it'll be feature flagged.
- I think we should show an example column name using the format tag, or maybe just the format tag itself in one of those click-to-copy boxes
For instance, for usd
, are you thinking of an example like select value as value_usd ....
?
- Can/do we do any validation on the format tags to make sure they aren't special characters or something that couldn't be used in SQL?
Currently, we are only allowing strings for the format tag name. You'll see an error if you attempt to use a different character. I think this rule could be relaxed to allowing and numbers as long as they are not leading characters. I believe our formatting logic looks only for formats after the last so we have to be careful here with underscores.
For instance, for usd, are you thinking of an example like select value as value_usd .... ?
Yeah, or even just something like a tag column with: _usd
. It'd be very repetitive with the name column though. Perhaps there's a way to flatten into one idea. I think something to think about as we use it.
On the example inputs, I think they're helpful to play around with and see what the formatted result would be. I agree about adding noise to the page though. Maybe we could make that column narrower by changing all the date example inputs to YYYY-MM-DD (+ time if applicable).
+1 on keeping them in -- the builtins are useful as a reference, and as an example for how to create new ones.
One other idea re: noise would be to put them in something collapsible so that you could reclaim the vertical space if you didn't want to look at them. Ultimately I think this settings panel should probably live on its own page as well.
@hughess
Maybe we could make that column narrower by changing all the date example inputs to
YYYY-MM-DD
(+ time if applicable).
Makes sense. I sort of tried to demonstrate with the input date examples that the input value can take a variety of formats, but I am not sure if it helps the user and it comes at the expense of cluttering the UI.
I think we should allow numbers to be a part of the custom format tags (e.g., if someone wanted to create variations of a custom format with different decimal places). I think leaving out underscores makes sense for now.
Yeah, I agree - we should allow non leading numbers. Underscores can be tricky given how we infer the format name (aka tag?) from the column name alias.
Is it possible to use quotes instead of escaping characters for strings inside the format codes?
Good eye. I tried it and it works. I'll fix all the relevant examples
I haven't dug into this yet, but I'm seeing a few charts with blank axes, I'm assuming because they threw an error during formatting - this one is on pages/charts/index.md
I haven't dug into this yet, but I'm seeing a few charts with blank axes, I'm assuming because they threw an error during formatting - this one is on
pages/charts/index.md
Thanks, I'll take a look. I though I didn't really change formatAxisLabels.js
semantically, but I may have done so inadvertently.
I haven't dug into this yet, but I'm seeing a few charts with blank axes, I'm assuming because they threw an error during formatting - this one is on
pages/charts/index.md
Fix here: https://github.com/evidence-dev/evidence/pull/325/commits/70f298e323ec625c6070a30f9a80081bee08ee04
Also needed to add the k/M/G suffix in back into post formatted numbers (done under different commits)
Last thing I noticed in the charts. I think formatTitle
isn't able to handle the input from the new formatter. For example, the axis titles in the chart below don't replace the _usd
in the column name with ($)
:
This is what I see if I log the column name and column format from formatTitle
:
formatTitle
Good catch. I think we'll have to pass in the format object going forward to the formatters.
i.e
getColumnFormat(colFmtTag)
would have to return the entire format object with the name/tag (e.g usd) with the expression (e.g $#,##0.00);
@hughess RE that issue where custom settings didn't appear in the component examples - had to do with a bug where customSettings weren't loaded when there are no queries : https://github.com/evidence-dev/evidence/pull/325/commits/ea7e64f3302b97f02ceb66a77bc821e4c7614bc4
You can enable this feature by simply adding a file named
sites/example-project/.custom-settings.json
with content:Example of how this works right now on the UI (you should be able to apply it by aliasing the format name in query): See https://github.com/evidence-dev/evidence/pull/325#issuecomment-1160726392
Also, I scoped creeped basic code formatting with prettier. Currently only meant to be run manually using
pnpx prettier --write <file-to-be-formatted>
. Errors may show up if you have the vscode prettier plugin installed, however this won't cause any build issues or preventing your from making merges.