Refactor How Column Style Components Are Selected #2443
Conversation
ada-x64
force-pushed
the
bugs/refactor-column-styles
branch
from
4a82995 to
88408db
Compare
There was a problem hiding this comment.
Thanks for the PR!
expect_throw() isn't something we should use in the middle of yew component tree evaluation I think. Your options here are (in order of preference):
- Prevent these cases in the type system if possible so they can be factored away at compile time. (sound like not the case here but mentioning it all the same).
- If this is not possible, render a blank stub and spam the console with warnings, plus a test for this functionality would be nice.
| .map(|ty| ty == Type::String) | ||
| .unwrap_or_default() | ||
| && label.as_deref() == Some("Symbol") | ||
| }, |
| </div> | ||
| </div> | ||
| } | ||
| {opt_html.expect_throw("Could not generate HTML to style this column!")} |
There was a problem hiding this comment.
Errors are not the right way to handle this (or really in general), not even JavaScript errors. It is really hard to predict when something that is the result of a long complex process will adhere to restriction like this that cannot (or at least, isn't) be represented in the type system. More importantly, throwing an error here can corrupt/crash the rust app.
There is a similar circumstance in active_column.rs, where there is a small corner case where the View must exists before the UI can be rendered. This render path shouldn't be hit, so we render a stub.
In this case, we probably want to keep the stub, we just don't want it ever to show. If for some reason it does, we don't want to segfault, so it needs to render something because we can't (AFAIK) prevent this logic from being called with a missing view type (?). Probably also worth warning the console as this is programmer error.
| @@ -132,7 +136,8 @@ pub fn ColumnSettingsSidebar(p: &ColumnSettingsProps) -> Html { | |||
| { renderer } | |||
| { custom_events } | |||
| { selected_column } | |||
| { on_close }/> | |||
| { on_close } | |||
| /> | |||
There was a problem hiding this comment.
We need to sync up on style guide for html! macro content, because it is immune to rustfmt and changes like these make it difficult for me to use file history to bisect issues.
ada-x64
force-pushed
the
bugs/refactor-column-styles
branch
2 times, most recently
from
549807d to
43dd4bc
Compare
ada-x64
force-pushed
the
bugs/refactor-column-styles
branch
from
43dd4bc to
2650f35
Compare
Co-authored-by: Tom Jakubowski <tom@crystae.net>
add fixture test
ada-x64
force-pushed
the
bugs/refactor-column-styles
branch
from
2650f35 to
33220af
Compare
|
This has been updated to add a stub, remove expect_throws, and add a fixture test. In order to add the fixture test, I rebased this on #2447 |
This PR does what it says on the tin. The goal is to prevent unnecessary style component renders and to replace the default "no styles available" content with a hard error.
There are some remaining issues with the symbol column settings when aggregating. In particular, the column value selector does not get the aggregated values, but the raw table values, so the key-value selector doesn't match up with what is in the plugin.