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: |