-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basic frontend collection management #230
Conversation
As of #181, /api/collections only retrieves the user's own collections by default. The .mine method is retained for easier porting and as an easy way to resume the distinction if it is added again in the future.
This enables the alt-click handler. Eventually, we may want to disable this again in production.
The fact that this was not correctly set yet is a historical artifact that apparently has gone unnoticed for a long time. It is unrelated to #181.
I think this clarifies the meaning.
Just to make the development work less straining on the eyes.
When I try this, no new entry is added to the menu but an exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful! I love the OverlayView
. I have just a few comments. I can pick up the things of which you mention that they are still missing in the time that you are away.
}, | ||
|
||
toggle: function() { | ||
this.cover() || this.uncover(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow!
@@ -40,11 +40,13 @@ export var ProjectMenuView = CollectionView.extend({ | |||
}, | |||
|
|||
select: function (model) { | |||
model = this.collection.get(model); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not very clear, I would change the variable name or add a comment
This had been broken for a long time. The fix ensures that a project is always selected, which also addresses the "rough edge" discussed in #230 (comment).
This had been broken for a long time. The fix ensures that a project is always selected, which also addresses the "rough edge" discussed in #230 (comment). CC @tijmenbaarda
5829c39
to
9e48842
Compare
While frontend collection handling forms the overarching story of this branch, it is a bit of a hodgepodge of small changes:
summary
of a collection visible through a<small>
insertion in the page header.idAttribute
of the Backbone model that represents a single VRE collection touri
.<small>
in the page header in order to adjust thesummary
of a collection.OverlayView
in order to support this. It can do one thing: temporarily replace an arbitrary DOM element by an arbitrary view, based on a selector relative to a persistent root element.OverlayView
instances).idAttribute
of theProject
model toname
. It appears that theidAttribute
was never set to a correct value before this..editorconfig
..gitignore
.headlessChrome
from the default browsers in the Karma config as discussed in Tests using GitHub Actions #200 (comment).GlobalVariables
, using thevreChannel
instead for loose coupling. This brings us a bit closer to Use backbone-fractal and backbone.radio where applicable #123 again.modelSlashUrl
from backbone-util.I will be on vacation soon, so I prioritized fast delivery over comprehensiveness. Hence, there are some omissions and rough edges that I am aware of:
but an entry with the new name still appears in the menu.There are also things that were already broken on
develop
and which are not fixed by this branch:For review, I suggest giving the frontend a quick spin and just scrolling through the code changes. There are lots of files with only minor changes, but I think you will be able to quickly identify repeating patterns and skip over those.