Tim Donohue [9:52 AM]
@here: a reminder that we'll be holding our first DSpace 7 UI text chat meeting via Slack (starting in this channel) at the top of the hour. This is a status update meeting, so there's no formal agenda, but relevant links are at: https://wiki.duraspace.org/display/DSPACE/DSpace+7+UI+Working+Group Tim Donohue [10:00 AM]
@here: It's DSpace 7 UI Meeting time! I'll get things started... [10:01]
First off, as a reminder, we'll be meeting for an hour. The first 30 mins will be in this channel & include updates on Angular UI work (over the last week [10:01]
The second 30 mins will be in #rest-api channel, where we'll do the REST API updates [10:01]
@art-lowel : would you like to get us started here with updates on Angular UI work? Art Lowel [10:01 AM]
ok [10:01]
The PR for the first version of the HAL serializer is ready
https://github.com/DSpace/dspace-angular/pull/61 [10:02]
It takes a model class as a param to its constructor [10:02]
e.g.: `new DSpaceRESTv2Serializer(Bitstream)` [10:02]
It can use any model that has @autoserialize annotations [10:02]
e.g. https://github.com/artlowel/dspace-angular/blob/rest-services/src/app/core/shared/dspace-object.model.ts#L14 [10:02]
The PR also contains a validator, that can use JSON schema to ensure documents fit the spec [10:03]
Weeding out malformatted responses before you start makes serialization easier, as you don't have to test whether everything exists before you do something with it [10:03]
JSON Schema is something like XSD for JSON [10:03]
I started with the default hal json-schema - we can expand on it later by adding rules more specific to dspace.
http://hyperschema.org/mediatypes/hal [10:03]
but our current mock responses don't fit that default hal schema (something about the structure of the `_embedded` element).
So the validator isn't used yet. [10:04]
Building on the serializer PR, there's also a PR that adds a CollectionDataService and an ItemDataService
https://github.com/DSpace/dspace-angular/pull/62 [10:04]
These services offer a `findById` method, and a `findAll` method with an optional `scope` param, for Items and Collections [10:04]
The PR *doesn’t* have all the required documentation and tests at the moment. [10:04]
I'd also like to refactor it, as there is too much code duplication between the Item and Collection Services, and their corresponding actions, reducers and effects. [10:04]
But these services can be used to start making UI components such as collection homepages and item view pages [10:04]
So I suggest that people review them as they are, and that we merge them ASAP so other work can start [10:05]
Their issues will stay open until docs, tests and refactoring are complete. Tim Donohue [10:05 AM]
Nice, sounds like some good progress here :wink: Art Lowel [10:05 AM]
do you agree/disagree with that approach? Tim Donohue [10:06 AM]
So, to clarify, we need to merge the HAL serializer first, #61 (ASAP). Then we'd merge the Collection/Item services (#62) and work to refactor them later? Art Lowel [10:06 AM]
yes [10:06]
the HAL PR should be up to snuff Bram Luyten (Atmire) [10:06 AM]
joined #angular-ui Art Lowel [10:06 AM]
the other one isnt Tim Donohue [10:07 AM]
That seems reasonable assuming the refactor work in #62 is still made a priority. It sounds like that's important and shouldn't be forgotten Art Lowel [10:07 AM]
I’d still like other people to test them first [10:07]
yes the issues will remain open until that is completed Hardy Pottinger [10:07 AM]
"merge first" = "enforced testing" :wink: Tim Donohue [10:08 AM]
@art-lowel : How does one go about testing the HAL Serializer (#61)? Is that one really just a code review (that we later test in #62)? Art Lowel [10:08 AM]
I just hurried to get something out there people could start developing against [10:08]
code review, and review of its unit tests Tim Donohue [10:08 AM]
makes sense Art Lowel [10:08 AM]
I made it against the unit tests [10:08]
but it worked in practice when I was working on #62 :slightly_smiling_face: Tim Donohue [10:09 AM]
Do we have a volunteer who wishes to help/perform a code review of PR #61? @wwelling is this of interest to you? I can also help, but might appreciate a second pair of eyes (my Angular is a tad rusty and needs brushing off) Art Lowel [10:11 AM]
this part has very little angular specific code in it btw James Creel [10:11 AM]
I have put a little time into the mock responses which need reconciling with the schema, so I may be of some service Art Lowel [10:11 AM]
If you’re good with java, you should be able to work it out Tim Donohue [10:11 AM]
Thanks @jcreel256. If you can help review #61, I'd appreciate it. I will also review it (though I have meetings all morning, and may not get to it till this afternoon) [10:12]
How about a volunteer here on testing #62 as well? Anyone interested in helping move along the Collection/Item services? Art Lowel [10:12 AM]
That one is more complex though [10:13]
although I tried to give somwhat of an explanation in the PR description Tim Donohue [10:13 AM]
Is there a recommended testing approach? Or is this another code review (for now), until we get UI components built out? Art Lowel [10:14 AM]
I’d say, use it: create a component that uses CollectionDataService.findAll for example [10:14]
and use it to list collections somewhere [10:14]
then use the redux dev tools to see what state changes occur Antoine Snyers [10:15 AM]
I’ll take a look at it :wink: Tim Donohue [10:15 AM]
Thanks @antoine , sounds great! I can again look at the code, but not sure I'll get to testing. So, please let us know what you find! [10:16]
Ok, we are 15mins into the meeting already. Other major updates to share while we are all here? Art Lowel [10:16 AM]
I've looked up why yarn was removed from the universal starter project [10:16]
The only mention I can find about this is: _“I"m going to remove yarn.lock since it causes too much merge conflicts”_ in this PR:
https://github.com/angular/universal-starter/pull/262#issuecomment-261736701 [10:16]
Looking in to it, merge conflicts with the lock file seem to be a problem with yarn, there is an open issue about it on github. [10:16]
But it is a problem inherent with the use of lockfiles, so it also affects npm-shrinkwrap [10:16]
Since the lock files for both tools are automatically generated, there's a chance they'll clash when two people independently add dependencies to them. [10:17]
For now the solution to this issue seems to be to take `origin/master`'s version of the lock file, and then running yarn install again. That way yarn will merge them for you. [10:17]
https://github.com/yarnpkg/yarn/issues/1776#issuecomment-269539948 [10:17]
Since npm shrinkwrap lockfiles have the exact same problem I don't see this as a dealbreaker for yarn. [10:17]
It will need to be automated as best we can in an npm script, and explained in the docs. Tim Donohue [10:17 AM]
+1 to docs / automation here [10:18]
It also doesn't sound like a dealbreaker to me. So, as there are benefits (and since yarn is "backwards compatible) I'm OK with us trying out Yarn Art Lowel [10:18 AM]
:y::skin-tone-3: [10:19]
last time @wwelling said something about using OpaqueToken for the config. [10:19]
I did some reading about it, and I can see the benefits of injecting environment variables. It makes it easier to mock them during tests for example. [10:19]
so that idea has my +1 Tim Donohue [10:20 AM]
Sounds good Art Lowel [10:20 AM]
but I’m not sure I understand the details well enough to write a task for someone else to do it [10:20]
so I wanted to ask @wwelling if he could write the issue, or implement it himself Tim Donohue [10:20 AM]
@wwelling or @jcreel256 , any chance you could help out here (writing a task or implementation)? [10:21]
We can touch base with them post meeting as needed. I don't want to wait too long for a response (in essence of time) :wink: Art Lowel [10:21 AM]
makes sense James Creel [10:21 AM]
excuse us, multitasking :slightly_smiling_face: Tim Donohue [10:22 AM]
Are there other updates (either from @art-lowel or anyone else here)? Or other questions/topics folks want to discuss regarding Angular UI? William Welling [10:22 AM]
I can create the task. Tim Donohue [10:22 AM]
thanks @wwelling! William Welling [10:22 AM]
I have some questions about the spinner feature. [10:23]
Do we still intend to have a spinner that intercepts route change and displays conditional on pending asynchronous requests? Tim Donohue [10:23 AM]
aforementioned "spinner" (PR): https://github.com/DSpace/dspace-angular/pull/43 William Welling [10:23 AM]
I recall saying a Guard CanActivate may be able to achieve this, but it may be easier than that. [10:24]
Yes, I am curious on the requirement of this feature. Art Lowel [10:24 AM]
Yeah, I believe last time we agreed you’d look in to it, and perhaps come up with an alternative William Welling [10:24 AM]
A single page app should have notification when pending data is being requested, but I dod not think any navigation should be affected. Art Lowel [10:25 AM]
The CanActivate would prevent you from navigating to the route before it was loaded? William Welling [10:25 AM]
The navigation should occur immediately displaying whatever is available. A notification should only be for the asynchronous requests. If we end up having to wait between navigations it will be a poor user experience. [10:26]
Such as selecting next page of a pagination or selecting a item from a list. The requests should be minimal and only retrieve data that is needed at the time and if it takes longer we should make other affordances. [10:27]
The practical use of a user affected spinner is creation, save, or update where the completion of a request is necessary for any further user interaction. Art Lowel [10:27 AM]
That’s a good point William Welling [10:27 AM]
@art-lowel yes, that is correct. But could activate and set a process that enables the spinner and waits for the one to complete its asynchronous tasks. James Creel [10:28 AM]
I've got to duck out for another quick meeting, and I will join the REST discussion a little late. Tim Donohue [10:28 AM]
As we are running short on time here, I wonder if it'd be worth considering creating a (discussion) ticket for this in GitHub, so that @art-lowel and @wwelling can discuss in more detail & come to consensus? (and others can take part too) Art Lowel [10:28 AM]
We can use the spinner issue that’s already there William Welling [10:28 AM]
Ok Art Lowel [10:28 AM]
https://github.com/DSpace/dspace-angular/issues/24 Tim Donohue [10:28 AM]
In general, I wonder if such detailed discussions (and even requirements gathering/clarification) could be done more in GitHub Issues? Hardy Pottinger [10:29 AM]
+1 github discussions Tim Donohue [10:29 AM]
It might make these text chat meetings go a bit more smoothly, as it means we can bring *proposals* here, rather than having all the discussions here :wink: [10:30]
But, it sounds like @wwelling has some good ideas here, I'd just recommend figuring out the final details/requirements in the ticket and come back with a proposal (as needed) [10:31]
@here: As it has now been 30 mins, the meeting will now move into REST API discussions in #rest-api channel. If there are further angular-ui discussions, you can still hang out here, but my attention will move to #rest-api Andrea Bollini [10:31 AM]
anyway, it is useful to have an half hour appointment to be sure that some discussion happen Tim Donohue [10:33 AM]
Yes, this mtg structure is a work in progress here...i'm sure these first few Slack mtgs will be a bit "rough", but hopefully it will smooth out a bit more as we learn how to fit discussions into time allotted. Nonetheless, feedback is welcome :wink: |