This pull request has been mentioned on Discourse Meta. There might be relevant details there:
Hi @merefield could you rebase on top of master? There is linting failures that I think were fixed in the latest release and it’s making it hard to review without them.
@eviltrout sure np, will revert. Also note that the new bookmarks.json fixture is a bit of a hack from latest.json. Might be good at some point to improve upon it if any tests get higher granularity. It would be good to use your test accounts data.
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
11 out of 13 committers have signed the CLA.
You have signed the CLA already but the status is still pending? Let us recheck it.
@eviltrout something still not right?
Yes it looks like there are prettier failures in
The best thing here is to set up your editor to automatically format the code. There is some info on that here
As for the feature itself, am I correct in saying it allows /bookmarks as the homepage? We have the much nicer new /my/activity/bookmarks page, and I seem to remember some talk about deprecating/removing
/bookmarks. @martin-brennan is that something you were thinking about?
Ooops apologies … perhaps my prettier config is out. I’ll take a look.
@davidtaylorhq thanks for the pointers, that resolved it and I should be free of that issue for future PRs now I’ve got the config right!
This looks good now but I’d like to hear @martin-brennan’s response before we merge.
@davidtaylorhq thanks for pinging me, at this time we are leaving the /bookmarks page as is because it serves a slightly different purpose to the other bookmarks list. I think adding this functionality is fine for now.
I am not sure why all this dark mode stuff is here, possibly merge/rebase shenanigans?
This works great, thank you for adding specs as well. I think its fine the fixture follows latest because the /bookmarks list doesn’t differ much from latest. If you just fix up the comment it should be good to merge.
Also in future could you please add a PR description of the what and why for the PR; I realise it is in Meta but having a succinct summary of what is changing helps a lot with reviews
Good catch. Yes a rebase issue. Will address and revert.
@martin-brennan removed the imposter test and embellished the commit message to better describe the change. thanks for your review!