Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

Custom Backgrounds #335

Merged
merged 15 commits into from
Oct 26, 2017

Conversation

krau612
Copy link

@krau612 krau612 commented Oct 10, 2017

Allow the user to upload custom backgrounds. Resolves #320

screenshot at 2017-10-10 16-42-36

screenshot at 2017-10-10 16-53-10

Copy link

@spresnal spresnal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider deciding on a standard for logging so log level can be set or manipulated when need. Whether it's trace/debug/info/critical logs we want to see. This'll prevent a lot of noise from console logs everywhere.

filters: [
{ name: 'Images', extensions: ['jpg', 'png', 'gif'] }
],
properties: ["openFile"],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary comma here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't see a problem with this. It's valid syntax see here. Also, it means it's 1 less line on a diff when an additional value is added at the end of the object.

Copy link

@spresnal spresnal Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The syntax is valid, however, the linter which we are trying to move to (StandardJS) flags it by default. See Trailing commas not allowed. here: https://standardjs.com/rules.html#javascript-standard-style

That being said, I do kind of like the idea of keeping the diff a bit cleaner.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all for a standard, so in that case I agree 😄

I've always done trailing commas on arrays to save on the diffs. I can't argue with standards though xD

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and removed the comma. I wasn't aware of the StandardJS style (and honestly I put it there on accident lol). Thanks guys!

properties: ["openFile"],
};
var userPath = 'assets/images/user/'
var dirPath = path.resolve(__dirname, userPath);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semicolon consistency (Need that linter ASAP @j-a-m-l )

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the semicolon. That was my bad. Thanks!

return;
});
writeStream.on("close", function(ex) {
$mdToast.show(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a good idea to pull the toast logic into its own method where you pass in the text to be displayed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. I went ahead and made a showToast function. Now the toasts are all made with either showToast or formatAndToastError. I also removed my unnecessary log statements. Thanks!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are already a few Toast function, but it is not rational:

Would be nice to revamp the toast management into a single identified service instead

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be for another PR

@luciorubeens
Copy link
Contributor

Nice, I'll upload many gifs 😁
Upload is ok, but the ui should be improved:

  • Hide the upload button in the themes tab.
  • Delete button must be next to each image (perhaps an icon near the top), so you don't need to select the image first to delete.
  • After delete/upload don't call the manageBackgrounds, just update the array.
  • After delete, update the background to default.

@krau612
Copy link
Author

krau612 commented Oct 24, 2017

@luciorubeens Thanks for the suggestions! I went ahead and implemented all of them. One issue though is that when clicking the delete button, which is now on the image grid tile, the image is selected for a split second before it is deleted. Do you know how I could fix this?

@luciorubeens luciorubeens merged commit 93da53f into ArkEcosystem:master Oct 26, 2017
luciorubeens added a commit that referenced this pull request Oct 26, 2017
@luciorubeens
Copy link
Contributor

Hmm I did not noticed but I added preventDefault().. good job! 👍🏻 +5

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants