AENV-69: As an author I want to add my article to a respective playlist #31
Conversation
|
@plamut pls review |
|
@takeit Is it urgent? I didn't manage to do it yet, unfortunately, it's been a busy day. |
|
@plamut no rush as there is one todo left, take your time. I pinged you to review that when you find some time. |
|
OK, good, I'll probably have some spare time tomorrow. |
takeit
changed the title
| _.remove( | ||
| self.assignedArticlesLists, | ||
| {id: value.id} | ||
| ); |
There was a problem hiding this comment.
Just checking - is it really safe to delete the elements from the array while iterating over it? Does it, for example, work for two consecutive elements when both should be deleted? It is likely that the second one gets skipped over if its immediate predecessor gets deleted.
I would recommend creating a test case for this and modify the code if needed. One possible solution would be to iterate over the array backwards.
(and if this is based on any of my code snippets - looks familiar - then I take the blame 😄 )
BTW, I think the comments need an update, too, since we are not adding and deleting lists, but the article objects instead?
There was a problem hiding this comment.
good point, although it worked fine, but it was not a good approach, I replaced that snippet with the iteration in reverse + I have added more tests to cover this
There was a problem hiding this comment.
since we are adding and removing featured article list(s) (not articles) from the array where those lists are being kept, comments are correct.
There was a problem hiding this comment.
OK, good then, leave the comments as they are then.
|
Looks good and readable so far, with docstrings and extensive test coverage, good work. |
|
Thanks for the review! I updated the PR according to your comments. |
|
I think the rest is fine, didn't find any issues really. It's also great to see that you guys still write readable code, keep doing that, it really benefits the project. |
featured articles lists optimizations added updating the existing assigned articles lists array iterate in reverse when removing the element fixed translations and less improvement
AENV-69: As an author I want to add my article to a respective playlist
This one should be merged instead of #29
TODO:
See an image below for the first two issues:
- [x] in the featured articles pane, there should be a pointer coursor when hovering the featured article list to which article is assigned to - [x] icon for the "Add existing articles list(s)" should be changed to "plus icon" ? at the moment there, is an image icon.
- [x] title attribute should be changed to describe featured article lists (atm there is text describing slideshows, e.g. Attach Slideshows etc) - [x] "Articles Lists" text should be changed to "Featured Articles Lists" - [x] "Close" button should be hidden in when Featured Article Lists modal is open (on the newscoop side) - add article's position in assigned featured list - **Note**: this will be introduced later.