Feature/download tab #1635
Feature/download tab #1635
Conversation
| this.totalResults?.complete(); | ||
| this.resultsConfig?.complete(); |
There was a problem hiding this comment.
Careful closing of the observables here, I wouldn't have thought to do so if replacing them with a new stream.
Out of curiosity, did problems arise when not closed?
There was a problem hiding this comment.
No, this is more "good practice", because we're creating observables in ngOnChanges, it's good to close them when new input comes in.
But this is triggered when the input receives a new queryModel. That's really an edge case. (See #1247 (comment) for more detail.)
| import { QueryModel } from '../models'; | ||
| import { mockCorpus } from '../../../mock-data/corpus'; | ||
| import { commonTestBed } from '../../common-test-bed'; | ||
| import { QueryModel } from '../../models'; |
There was a problem hiding this comment.
The imports for models and services would benefit from path mapping in tsconfig.json
| if (state.tab === 'search-results') { | ||
| return { tab: null }; | ||
| } else { | ||
| return state; | ||
| } |
There was a problem hiding this comment.
Familiarizing myself with the store stuff, which does look very neat.
I wonder if something like a 'default state' would be beneficial to add as an option in StoreSync? In this case if the tab is search-results.
There was a problem hiding this comment.
Hm, good point. Filters have something like that, but perhaps it could be generalised to more models.
This moves the download menu into a separate tab.
The download tab includes a form with all options for the download; it replaces both the "download" button on the top (with the attached dropdown menu) and the options dialogue window. (The options dialogue is still used in the download history, though.)
close #1310
close #1029
close #674
close #1118 (this suggested an alternative approach)
Screen capture:
Screencast.from.19-07-24.12.12.12.webm
Notes:
tabparameter. Old routes will continue to work.tagsandcontextas forbidden field names. This isn't because of any changes in this PR, I just noticed they should have been added.