Move "print" job logic and management out of area composable#737
Move "print" job logic and management out of area composable#737
Conversation
Passing run #1339 ↗︎
Details:
Review all test suite changes for PR #737 ↗︎ |
|||||||||||||||
e8b5613 to
e91646a
Compare
e91646a to
fce4af7
Compare
ismailsunni
left a comment
There was a problem hiding this comment.
Really much better. Thanks!. Only some small questions.
| // for now, we only check if app is ready and printing status | ||
| // TODO: any pending request to the backend should trigger the loading bar | ||
| const shouldLoadingBarBeVisible = !state.app.isReady || state.print.printingStatus | ||
| const shouldLoadingBarBeVisible = !state.app.isReady |
There was a problem hiding this comment.
If the printStatus is removed, how will the loading bar be shown?
There was a problem hiding this comment.
I should add a todo in the usePrint composable to tackle that.
The idea is that the composable itself will dispatch something (the source it is waiting on) and then subsequently dispatch once more when it is done. The loading bar will be shown whenever a source is not done loading, meaning we will be removing this loading bar management plugin altogether.
| const printLayout = computed(() => store.state.print.selectedLayout) | ||
| const printScale = computed(() => store.state.print.selectedScale) | ||
| const attributions = computed(() => | ||
| store.getters.visibleLayers | ||
| .map((layer) => layer.attributions) | ||
| .map((attribution) => attribution.name) | ||
| .filter((attribution, index, self) => self.indexOf(attribution) === index) | ||
| ) | ||
| const layersWithLegends = computed(() => | ||
| store.getters.visibleLayers.filter((layer) => layer.hasLegend) | ||
| ) | ||
| const lang = computed(() => store.state.i18n.lang) | ||
| const projection = computed(() => store.state.map.projection) |
There was a problem hiding this comment.
For these computed, should we just retrieve once when we call the function to prevent impact on performancce? e.g. only retrieve from store when we call createPrintJob?
There was a problem hiding this comment.
That might be a good idea yeah, I'll try and see if it works as intended
| await abortCurrentJob() | ||
| } | ||
| printStatus.value = PrintStatus.PRINTING | ||
| await store.dispatch('generateShortLinks', dispatcher) |
There was a problem hiding this comment.
Previously, @ltshb suggested me to call the function directy without saving the result to avoid a dispatch. #658 (comment)
There was a problem hiding this comment.
That is sound, then I'll move the part of the logic that decide to remove geolocation out of the store, in some function in shortLink.api.js so that we can re-use it.
I used the dispatch so that this part of the code wasn't duplicated (removing the geolocation URL param and so on)
The print area preview composable was doing the preview, the printing and also coffee. It is now only responsible for the preview area. I also took the opportunity to remove from the store things that weren't making much sense being there, as no one else as the print menu section was using them (and as the menu section is now the one triggering the print job, it's much clearer this way)
fce4af7 to
e195f54
Compare
e195f54 to
d836bed
Compare
The print area preview composable was doing the preview, the printing and also coffee. It is now only responsible for the preview area. I also took the opportunity to remove from the store things that weren't making much sense being there, as no one else as the print menu section was using them (and as the menu section is now the one triggering the print job, it's much clearer this way)
Test link