-
Notifications
You must be signed in to change notification settings - Fork 17
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
Experiment: format directory names with s/^
(home)/~/g
#410
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.
It's a nice idea. As with any nice UI feature, it's always harder to do well then you'd first thing :)
I left a variety of comments. I'd encourage you to (A) use Storybook! (B) consider how far you want to take the feedback I provided regarding lifting up the async request.
@@ -0,0 +1,15 @@ | |||
<script lang="ts"> |
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.
Let's use a naming convention for these readonly "formatted" components. One such convention I've seen a few times in the past is a "Label" suffix. Besides some precedence for it, it is shorter than "Formatted", and being a suffix means that it sorts with related components like *Field or *Form or whatever.
So then this would be "DirectoryLabel".
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.
Agreed, that sounds preferable.
<script lang="ts"> | ||
import { api } from '../lib/api'; | ||
|
||
export let raw: string; |
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.
For components following conventions such as *Label or *Field, we should also conform to a standard interface. This will later better enable generic programming. For instance, if we want to dynamically display a label based on type, we can substitute just the component itself and reuse any potential parameters passed to that component.
The convention here is set for us by html/react/etc: Name this property simply "value".
</script> | ||
|
||
<span title={raw}> | ||
{#await api.kernel.getUserHomeDir()} |
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.
Generally, we should hoist asynchrony as high as possible. Just as we want to separate template/render-only components from stateful containers/providers, even otherwise render-only components that perform IO should be careful to isolate that IO.
In particular, my concern here is that if someone uses this component to display a list of paths, it will cause a cascade of requests!
The simple fix is to define let home: string | undefined = undefined
and then pass it in from a call site. This will make the call site verbose and lift the problem one level up, but that's generally desirable.
A secondary issue is: What if we want to display absolute paths so that they are easier to copy paste to places that don't necessarily support ~
? By omitting home=
, the component becomes more versatile in that way.
An alternative approach would be to use a Svelte "store", which would prevent a cascade of requests. However, this too should be pushed up the component hierarchy. Because both the inline async and a global use of a store means that this component it not usable in isolated tests like Storybook.
Probably the best approach would be let home: string | null | undefined = undefined
and then use the Context mechanism. If home is null, do not render the ~. If home is undefined, check for that information in the component hierarchy via context. If home is provided, use that and ignore context. This way means that most call sites are unadorned with home=
, you get the automatic/default behavior, can still test the component where home directories are not provided such as in Storybook, and lift the async request logic way up high in the component hierarchy.
<span title={raw}> | ||
{#await api.kernel.getUserHomeDir()} | ||
{raw} | ||
{:then homeDir} |
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.
Another reason to push async higher up in the component hierarchy is because error handling is hard! If the getUserHomeDir call fails for any reason (for instance, the exo daemon may have crashed), then you're not retrying it and there is no :catch
block, so you'll render nothing!
Hey @jwmza, I'm going to close this. Feel free to re-open if you want to put in the extra elbow grease to get this in to shape. |
Not sure this is actually worth doing (at least, in this manner) but this changes a path like
/home/john/path/to/project
to~/path/to/project
which removes a lot of repetitive, likely unimportant, information from some locations (as in theHome
page).Having to call the API each time for each formatted directory seems like a bad solution though.