-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add ability to change the background colour of the Memory when creating it #10
base: master
Are you sure you want to change the base?
Conversation
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.
Good idea, I left you some comments :)
src/App.svelte
Outdated
@@ -8,6 +8,7 @@ | |||
let createMemoryIsOpen = false; | |||
let appInstalled = window.matchMedia('(display-mode: standalone)').matches; | |||
let installEvent = null; | |||
let cardColor = ''; |
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.
This variable is never used
src/App.svelte
Outdated
@@ -85,7 +86,7 @@ | |||
</button> | |||
</div> | |||
{#if createMemoryIsOpen} | |||
<CreateMemory save={saveMemory}></CreateMemory> | |||
<CreateMemory save={saveMemory}> </CreateMemory> |
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.
Unnecessary space
src/App.svelte
Outdated
</style> |
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.
Give me my line ending back 😆
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.
This is something that may happen from time to time with my setup but I should have spotted it on the git diff. I've been truncating them so far but forget this time. Out of interest, any reason not to have the newline or is it just preference?
src/CreateMemory.svelte
Outdated
@@ -1,29 +1,50 @@ | |||
<script> | |||
import Modal from './Modal.svelte'; | |||
export let save; | |||
export let cardColor; |
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.
export
is used when you want to pass a variable from parent component to children. This component creates this variable so export
isn't needed.
src/CreateMemory.svelte
Outdated
|
||
cardColor = newRandomColor() | ||
function changeColor() { | ||
var colorElem = document.querySelector("body > main > div.modal.svelte-7jwz2f > article") |
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.
This -7jwz2f
hash can change on its own when compiling.
This function should look like this:
function changeColor() {
cardColor = newRandomColor();
}
styles will update automatically
src/Modal.svelte
Outdated
</style> |
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.
Line ending eaten :)
src/CreateMemory.svelte
Outdated
</button> | ||
<button class="memory-form__color" on:click={changeColor}> | ||
<i class="memory-form__color-icon material-icons">palette</i> | ||
<span>Change Colour</span> |
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.
American English spelling is used throughout the entire project (in the code at least) so I would rename it to color
Think that covers the changes but if not I'll resolve the rest as soon as I can. Thanks for the valuable feedback. |
I will look at this in my free time, I'm adding hacktoberfest-accepted label for now |
I thought being able to pick the background colour of the memories would be a good feature as then people can colour code them as they please e.g. all angry memories have a red background.
This PR adds a 'Change Colour' button underneath the 'Save' button which will change the background to a random colour (it uses the original colour list at the moment) and, if the memory is saved, save this value so that every time it is opened it is in that colour. The 'Confirmation dialogues' will be red (which is set as a default).