Frontend Scaffolding + Helpers #7
Conversation
| "@typescript-eslint/explicit-function-return-type": 0, | ||
| "@typescript-eslint/strict-boolean-expressions": 0, | ||
| "@typescript-eslint/promise-function-async": 0 |
There was a problem hiding this comment.
I actually kinda liked these, and adapted all the Azure login code to match them. I guess they can be a bit overbearing (esp strict-boolean-expressions), but they do encourage good code on the whole
There was a problem hiding this comment.
I think they're pretty significantly overbearing: each of these is requiring that the user make explicit something that the compiler checks correctly. I started down the path of writing code for them, and they're way too much.
- Example for Explicit return type: composables have to have a thorough, complete, return type.
- Strict boolean expressions: if you want to check if a string is "", null or undefined, you now need to make 3 calls - it doesn't allow
if (!!s), which feels like a significant loss - Promise functions async - all calls to
withLoadingAndErrorHandlinghave to have an explicit async argument, turning a nice arrow function into an awfulasync () => await fooBar()
There was a problem hiding this comment.
A ~650 KB gif is still heresy, same comment from the last time about webp and other web video standards.
There was a problem hiding this comment.
I'll add a TODO for this - I looked into this before and there was a nontrivial lift to make sure browser support was there (with fallbacks, etc).
frontend/components/modal/Error.vue
Outdated
| Some common troubleshooting steps that might be helpful: | ||
| <ul> | ||
| <li><b>Refresh this page</b> - most of our pages save your progress as you go, so it's almost always fine to reload the page.</li> | ||
| <li><b>Check your internet connection</b> - this site requires connection to the internet for most functionality.</li> | ||
| <li><b>Visit this site on a desktop computer</b> - this site works best on desktop web browsers.</li> | ||
| </ul> | ||
| If this issue persists, please report this issue by <a | ||
| href="https://github.com/RMI/opgee-api/issues/new" | ||
| target="_blank" | ||
| >filing a bug in the OPGEE repository</a>. | ||
| </div> |
There was a problem hiding this comment.
Fix all the OPGEE references
| <StandardContent> | ||
| <TitleBar title="New PACTA Version" /> |
There was a problem hiding this comment.
Same note here as in StandardContent, every page shouldn't have to specify these components, we can hoist them into the default layout and set the title with a useTitle composable or similar.
There was a problem hiding this comment.
I disagree on this one - mostly because is likely to contain other stuff (like a 3-dot menu) that benefits from being set up as a normal component (i.e. access to computed values in it's props)
Aside from the one issue with OAPI spec discussing offline, this PR is ready for reveiw. What does it include?
And, an initial editor for PACTA version definitions, including edit, delete, etc. Will need a bit more work after the OAPI issues are resolved.
Feel free to review!