extract shared code into DocumentList #275
Conversation
|
@SimonSimCity I'm still working on implementing the "See all" pages. But you can already review this part. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #275 +/- ##
==========================================
- Coverage 10.47% 10.36% -0.12%
==========================================
Files 97 101 +4
Lines 4497 4545 +48
Branches 181 185 +4
==========================================
Hits 471 471
- Misses 3934 3978 +44
- Partials 92 96 +4 ☔ View full report in Codecov by Sentry. |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-meadow-01b17e903-275.westeurope.3.azurestaticapps.net |
There was a problem hiding this comment.
I've now reviewed it as if it was a normal PR 😅 Apart from these changes, I'd merge it straight away. Codewise I only see an improvement, and some conceptual arrangements which someone not familiar with deeper concepts would have to read himself into (not for time-to-time contributors).
…document-models # Conflicts: # pages/index.vue
|
@SimonSimCity I also need to update bmm-sdk-fetch since I found 2 bugs in the definition when working with this PR. Until that happens the linter will fail. |
|
@kkuepper Currently the linter fails because:
The reason for If you want, we can elaborate about it - as I don't quite know the concept you're trying to set here, I guess - that's at least what I read off the responses you wrote to my comments ... |
Updating bmm-sdk fixed that issue. |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-meadow-01b17e903-275.westeurope.3.azurestaticapps.net |
|
@SimonSimCity @sifferhans It's ready for final review / merge. |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-meadow-01b17e903-275.westeurope.3.azurestaticapps.net |
components/DocumentList.vue
Outdated
| watch( | ||
| props, | ||
| () => { | ||
| toolbarTitleStore().setToolbarTitle(props.list?.title ?? " "); |
There was a problem hiding this comment.
Do all pages who use this for listing set the title here? My suggestion is that the title should rather be handled by the page than by a component.
There was a problem hiding this comment.
Not sure what you mean. If the server returns a DocumentList it also provides the title.
To make it obvious on the page that the title will be set, I could create a method SetTitleFromDocumentList() or so.
There was a problem hiding this comment.
Well... I'd rather put these lines into each page... For me, the title is a part of the page and should not be controlled by a component. By this, you can use this component at more pages, also on one page multiple times.
pages/browse/index.vue
Outdated
| </template> | ||
| </ul> | ||
| <template v-else> | ||
| <DocumentModels v-if="models !== null" :models="models"></DocumentModels> |
There was a problem hiding this comment.
Why not use the component DocumentList here?
There was a problem hiding this comment.
I don't understand the question
pages/index.vue
Outdated
| </template> | ||
| </ol> | ||
| </template> | ||
| <DocumentModels v-if="models !== null" :models="models"></DocumentModels> |
There was a problem hiding this comment.
Why not use the component DocumentList here?
| toolbarTitle.value = title; | ||
| } | ||
|
|
||
| return { setReactiveToolbarTitle, toolbarTitle, setToolbarTitle }; |
There was a problem hiding this comment.
This implementation is prone to the problem, that the title will be overwritten if the reactive title is updated late.
utils/reactiveApi.ts
Outdated
|
|
||
| export default function reactiveApi<Data, Error>(data: AsyncData<Data, Error>) { | ||
| const stopHandler = watch( | ||
| () => contentLanguageStore().contentLanguages, | ||
| [ | ||
| () => useNuxtApp().$i18n.locale, |
There was a problem hiding this comment.
that probably doesn't work
SimonSimCity
force-pushed
the
feature/document-models
branch
from
ec97234 to
2e2f536
Compare
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-meadow-01b17e903-275.westeurope.3.azurestaticapps.net |
kkuepper
changed the title
No description provided.