search

commons-app / commons-app.github.io

Website for the Android Commons app
https://commons-app.github.io
Apache License 2.0
17 stars 27 forks source link

Create initial campaign editor #42

Closed domdomegg closed 5 years ago

domdomegg commented 5 years ago

A very basic campaign editor - just barebones at the moment. Will continue working on it, but hoping to get feedback early.

Things I can see need doing (and there are probably more):

Available at: https://domdomegg.github.io/commons-app.github.io/campaigns.html

Related: #35

sivaraam commented 5 years ago

Hi @domdomegg Great job with the simple campaign editor! :fireworks: :tada: Look forward to reviewing the code. Couldn't find time to dive into reviewing. Hopefully will let you know by this weekend. In the mean time, a few things that I noticed when using the editor hosted at https://domdomegg.github.io/commons-app.github.io/campaigns.html :

Import from GitHub

The option seems to be working fine.

Date picker

20190617_115449

Can you look into this?

Buttons in mobile

The 4 buttons seem to be breaking the single-pagedness in mobile.

Screenshot_2019-06-17-11-53-41

Can you check if there is a way to show them in a way that doesn't break the single pagedness. May be break them into sets of two { ("Add Campaign, "Remove campaign"), ("Import from paste", "Import from GitHub") }

Remove campaign

Currently the "Remove campaigns" button seems to be removing just the last campaign. So, it could be named "Remove last campaign". That said, it would be more useful if there was an option to remove individual campaigns instead of just the last one. That would make it more useful along with the "Import from GitHub" option to easily remove existing campaigns.

That's it for now. Will dig more and let you know :slightly_smiling_face:

domdomegg commented 5 years ago

Import from GitHub

The option seems to be working fine.

  • Better name: may be would could use a better name to avoid confusion. Some people might possibly misunderstand it as a feature to import from any GitHub URL. Something that conveys the meaning of "Import campaigns.json from the main campaigns repository" in a concise way would be good. May be leaving the name as it is and explaining what it does in with a title attribute would do?

I agree it could be misunderstood to mean any GitHub URL. I've changed it to 'Import > Live version' as it's shorter and I think conveys the important part - if you have other suggestions on what to call it do tell me.

Do you think it'd be useful to have a URL import option? It would be very easy to implement the way the code is structured.

  • Better handling of existing content: For now "Import from GitHub" seems destructive i.e, any existing campaigns added by the user seems to be dropped off without any warning. Warning them might be helpful. Alternatively, merging the contents instead of dropping-off the existing ones might be even more helpful.

I think merging the contents will be tricky. I've added a warning when importing that allows the user to cancel. It currently reads "Warning: importing will overwrite the current data. Do you want to proceed?", but happy to any suggestions.

Date picker

  • No date picker in Firefox: The date picker seems to be working fine in Chrome and Firefox for Android but it doesn't seem to be working in the desktop version of Firefox (on Ubuntu16.04). The following is what I see when I tap on the date field:

20190617_115449

Can you look into this?

I can't seem to reproduce this. It is just using the built in browser date picker. Can you check you're running the latest Firefox (I believe there was a bug in Firefox v62 that had this issue), and see if that resolves this?

Buttons in mobile

The 4 buttons seem to be breaking the single-pagedness in mobile.

Screenshot_2019-06-17-11-53-41

Can you check if there is a way to show them in a way that doesn't break the single pagedness. May be break them into sets of two { ("Add Campaign, "Remove campaign"), ("Import from paste", "Import from GitHub") }

I've made it so the buttons wrap if too wide, and shortened them to 'add', 'remove' and 'import'. Also, now there is individual removing we could get rid of the 'remove' button there.

Remove campaign

Currently the "Remove campaigns" button seems to be removing just the last campaign. So, it could be named "Remove last campaign". That said, it would be more useful if there was an option to remove individual campaigns instead of just the last one. That would make it more useful along with the "Import from GitHub" option to easily remove existing campaigns.

Done.

That's it for now. Will dig more and let you know 🙂

Thanks so much for the review, would love feedback on changes + any other ideas you come up with :)

Also I'm thinking it might actually be better in its own repository. We can still host it at commons-app.github.io, by naming the repository correctly (just like how I can host it at domdomegg.github.io) - something like campaign-editor and enabling Github pages will result it in being hosted at commons-app.github.io/campaign-editor.

sivaraam commented 5 years ago

Great job @domdomegg! The update seems good :slightly_smiling_face: A few more thoughts inline.

I agree it could be misunderstood to mean any GitHub URL. I've changed it to 'Import > Live version' as it's shorter and I think conveys the important part - if you have other suggestions on what to call it do tell me.

The name is fine. May be adding a 'title' attribute to explain that the data would be imported from the campaigns repository would be helpful.

Also, the 'Import from paste; option seems confusing. Given that it's a single line text box, I thought I was supposed to enter a URL to a paste of some kind e.g., GitHub Gist. It's only after some time did I realise that I had to paste the actual JSON itself in the textbox. May be using a text area like the one you use to add a comment in GitHub would have given me a hint that I have to paste the actual JSON contents.

campaign-editor-from-paste

See if you could instead use Bootstrap form elements (alongside modals?) to have a textarea (reference) for the paste.

Do you think it'd be useful to have a URL import option? It would be very easy to implement the way the code is structured.

It might be useful but I suppose that might actually add to confusion as to what URL works. We could clarify that. But, we might have to identify the invalid cases which are huge and avoid them. May be we could start simply by accepting URL to Raw GitHub user content from files and gists? I think that would be useful. That's just my opinion. We could start with something else if it might be more easier :wink:

I think merging the contents will be tricky. I've added a warning when importing that allows the user to cancel. It currently reads "Warning: importing will overwrite the current data. Do you want to proceed?", but happy to any suggestions.

It's fine for now. We could look into merging later. Just a cosmetic suggestion, instead of using 'confirm()' see if you could use some bootstrap modal for the confirmation as it would look more natural (example implementation). I'm fine for now without this too :wink:

Date picker

I can't seem to reproduce this. It is just using the built in browser date picker. Can you check you're running the latest Firefox (I believe there was a bug in Firefox v62 that had this issue), and see if that resolves this?

It seems to be working fine for now. I'm currently in Firefox 67. I'm not sure which version I was on when I tested and reported it :man_shrugging: I don't think it was as old as 62 :thinking:

Buttons in mobile

I've made it so the buttons wrap if too wide, and shortened them to 'add', 'remove' and 'import'. Also, now there is individual removing we could get rid of the 'remove' button there.

Yeah, we could just remove the 'Remove' button as we can remove each campaign individually. Or we could have a 'Remove all' button to remove all campaigns after a confirmation? Not sure if it's would be useful enough, though :man_shrugging:

Remove campaign

Done.

Looks good :slightly_smiling_face:. Just a cosmetic suggestion, it would be nice if the button was smaller and was located in the center / right end. The big button looks a bit odd.

campaign-editor-remove-button

That's it for now. Will dig more and let you know slightly_smiling_face

Thanks so much for the review, would love feedback on changes + any other ideas you come up with :)

You're welcome :slightly_smiling_face: Just thinking out loud, making Undo / Redo work would be a great thing to have but would be out-of-scope for the bare-bones editor :wink:

Also I'm thinking it might actually be better in its own repository. We can still host it at commons-app.github.io, by naming the repository correctly (just like how I can host it at domdomegg.github.io) - something like campaign-editor and enabling Github pages will result it in being hosted at commons-app.github.io/campaign-editor.

You're right. It would be nice to keep it in it's own repository and keep this repository as just a "Landing page for Wikimedia Commons Mobile App". But for this we would need the help of @misaochan

When can this be done? After the review for this PR is complete or can we do it right away?

misaochan commented 5 years ago

Sure, happy to create a new repo for this in our org. Just to clarify, we want the name of the repo to be campaign-editor, right?

sivaraam commented 5 years ago

Sure, happy to create a new repo for this in our org. Just to clarify, we want the name of the repo to be campaign-editor, right?

May be campaigns-editor would be a more appropriate name? :thinking:

domdomegg commented 5 years ago

I've created campaigns-editor, but we can always rename this if necessary. Is it okay if I start pushing stuff there (continuing from what I've done here) - it will be live at commons-app.github.io/campaigns-editor. Alternatively I could disable Github pages so it's not live but the code is public on that repo, or I can just make a PR to that repo but that does mean that others can't add to it as easily.

sivaraam commented 5 years ago

Is it okay if I start pushing stuff there (continuing from what I've done here) - it will be live at commons-app.github.io/campaign-editor.

... Or may be we could just finish off the PR here continuing where we left off and move the code there afterwards?

I'm okay with moving there too. So, I'm not sure :man_shrugging:

Alternatively I could disable Github pages so it's not live but the code is public on that repo, or I can just make a PR to that repo but that does mean that others can't add to it as easily.

I'm not sure what you mean in the second part. Why would someone find it difficult to add to a PR? :thinking:

domdomegg commented 5 years ago

I'm not sure what you mean in the second part. Why would someone find it difficult to add to a PR? :thinking:

My bad, think I was wrong - not quite sure what I meant by that myself now!

I've moved over to the git repo https://github.com/commons-app/campaigns-editor so am closing this PR.

It's published at https://commons-app.github.io/campaigns-editor

misaochan commented 5 years ago

Thank you for your work, @domdomegg and @sivaraam ! :)