Scatterplot fixes and improvements #11898
Conversation
🧪 Storybook is successfully deployed!📊 Dashboard:
|
marthasharkey
requested review from
jdunkerley,
radeusgd,
GregoryTravis,
AdRiley,
Frizi,
farmaazon,
vitvakatu and
kazcw
as code owners
| ## PRIVATE | ||
| Colour |
There was a problem hiding this comment.
I feel like having both constructors Color and Colour is quite confusing. Which one should I use? Are they equivalent? Looking at the code below:
Point_Data.Color ->
colour_col = Point_Data.Colour.lookup_in table
before I noticed there's 2 different constructors I was pretty sure this was a typo (once Color and once Colour). It's so so easy to confuse the two.
Wouldn't it be much cleaner to keep a single constructor but modify the lookup/fallback mechanism to accept columns with both names? E.g. we can keep only the Color constructor but allow it to detect columns named color as well as colour. I would very much prefer such solution if possible.
There was a problem hiding this comment.
Yeah I agree it isn't clear, I'll look into how we can see if it will detect both!
| Point_Data.Color -> | ||
| colour_col = table.lookup_ignore_case "Colour" | ||
| is_good c = (self.is_recognized c).not | ||
| if is_good colour_col then colour_col else Error.throw Nothing |
There was a problem hiding this comment.
Looks much better.
Btw. not directly related to this change but I see that in this method, if the column is not found, we sometimes throw Nothing and sometimes throw No_Fallback_Column. Signature suggests No_Fallback_Column, so if possible would be good to get this consistent (although tbh it's not that important as long as it doesn't break anything since it's just internal code - still would be good to make it clearer if possible).
| const textSelectionButton = createTextSelectionButton() | ||
| return computed(() => [ | ||
| { | ||
| icon: 'select' as Icon, |
There was a problem hiding this comment.
as is not necessary here--there are typesafe ways to prevent the typechecker from widening these icon properties to strings. One approach would be to declare the return type, e.g. computed<ToolbarItem[]>(...)
marthasharkey
added
CI: No changelog needed
Pull Request Description
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.