Skip to content
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

A small improvement #3

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

JonesJugHead
Copy link

Lore update :

  • Addition of a preview for saved lores
  • Addition of a delete button for saved lores
  • Disable spell check on input
    image

@cheeezburga
Copy link
Owner

This looks like a really nice addition, thanks for the PR! I'll just do some testing with it when I get home before I merge it 👍

@cheeezburga cheeezburga added the enhancement New feature or request label Nov 18, 2024
Copy link
Owner

@cheeezburga cheeezburga left a comment

Choose a reason for hiding this comment

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

Did some quick testing and I seem to get this whenever there's only 1 line:
image

Also, just some other small notes:

  • Would rather comments are left out, I'm planning on going through everything and adding comments later on
  • If you wouldn't mind using ' instead of "

@JonesJugHead
Copy link
Author

Here it is, I've updated it.

I admit that my Prettier (https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode) wants to fix a lot of things
(Tab to space, " to ', etc., but there is no configuration file in the project, so I don't know the standards to follow. It would be good to add a Prettier configuration file and format all the files accordingly).

Regarding the buttons, I admit that the way you've done them isn't extremely good. If you want, we can merge the PR, and later in the evening (GMT +1 timezone), I'll submit a PR where I fix your HTML a bit and the way things work (HTML & CSS).

@@ -184,7 +184,7 @@ document.addEventListener('DOMContentLoaded', function () {
return '';

let code = 'set lore of {_item} to ';
const formattedLines = loreLines.map(line => `"${line.replace(/"/g, '""')}"`);
const formattedLines = loreLines.map(line => `'${line.replace(/'/g, '''')}'`);
Copy link
Owner

Choose a reason for hiding this comment

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

This is actually changing the functionality

@cheeezburga
Copy link
Owner

Here it is, I've updated it.

I admit that my Prettier (https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode) wants to fix a lot of things (Tab to space, " to ', etc., but there is no configuration file in the project, so I don't know the standards to follow. It would be good to add a Prettier configuration file and format all the files accordingly).

Regarding the buttons, I admit that the way you've done them isn't extremely good. If you want, we can merge the PR, and later in the evening (GMT +1 timezone), I'll submit a PR where I fix your HTML a bit and the way things work (HTML & CSS).

I'll look into making a Prettier config, wasnt aware of that extension. At the very least, I'll make a readme and include some of the basic styling stuff in there. And I think I'd rather merge a fully working version rather than two PRs, so if you want to just include any further changes in this one, feel free!

@JonesJugHead
Copy link
Author

Alright, I'll look into it then.
But yes, Prettier would be really cool, especially when we're collaborating.
Could you also add to the readme that using Prettier is recommended?

@cheeezburga
Copy link
Owner

Alright, I'll look into it then. But yes, Prettier would be really cool, especially when we're collaborating. Could you also add to the readme that using Prettier is recommended?

I've added a Prettier config file, and updated all files so far to use it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants