Vue Route in demo App #183
Conversation
|
sadiqkhoja
force-pushed
the
features/ui/vue-router
branch
from
604aa3c
to
e49a87e
Compare
There was a problem hiding this comment.
I'm excited to get this in. I do have several changes to suggest, on the review/features/ui/vue-router branch (see the diff), with each suggestion and reasoning in a separate commit. Feel free to cherry pick as you like, or make changes with adjustments as you see fit!
I know the separate branch/diff isn't necessarily an ideal format for review. I was finding it difficult to make some of the suggestions inline, and to make some it clear where some related changes would be implied across modules or with larger gaps between affected lines within the same module.
Hopefully none of these are particularly controversial. Looking forward to getting this in. I think it'll reduce a lot of little daily paper cuts that the manual history management approach has.
- We should consider the `optimizeDeps` change regardless. We do this in all of the other packages. - We should probably not make the changes to `showPopover` and `hidePopover` calls without deeper consideration of fallback behavior, if we are in fact supporting weird edge cases where otherwise-supported browsers are mysteriously lacking certain expected APIs Much more detail about the latter in getodk#211
…ped globals Note that `RouterLink` is already imported, so it’s superfluous in the ESLint config regardless.
We’ll be better off when we [automate Vue script/style code formatting](getodk#210), but this helped me better understand the styling structure
Not a huge deal, but it has some really surprising results with the fixtures as they’re currently named. It’s worth considering pulling the titles out of the fixtures themselves (this is done in ui-solid). My hesitation recommending this is that it slows down page load as we add more/larger form fixtures. I plan to introduce a change later which will make fixture loading asynchronous, but pulling the titles out of them for the listing isn’t trivially compatible with that.
All of this state and logic is reinventing functionality already provided by HTML, and well-supported by all target browsers. In general, let’s try to lean into the platform when it has perfectly good slutions to problems we’re working on!
It is very difficult to tell **what a thing is** when we frequently use the same name form different things with different meanings in different contexts. In many cases, we benefit from TypeScript type annotations to reduce this confusion, but it’s especially challenging where types are inferred. This naming is intended to match the same format used by `categoryName`. Symmetry of naming for like things is also a very helpful device for improving code comprehension!
The hover styles for the whole block were super confusing to me in a lot of ways. I do think there’s some value in a hover style to emphasize their clickability. But I think it makes much more sense to only style the category name for that purpose.
This consistency would already have been preferable. It now also simplifies: - Categorization of fixtures in FormList.vue - Route handling in FormPreview.vue
Same reasoning as earlier commit changing `form` -> `formName`
Please let’s not make it harder to debug when unexpected things happen
|
Thanks for the review, I have merged all the changes you suggested. |
The existing mechanism of using History API to handle routing is not foolproof, I noticed it breaks when demo app is run from a subdirectory. Hence, I have install vue-router. This was also suggested in #164 (comment)