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.

Sign up for GitHub

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

Jump to bottom

Updates to Install and Rules pages #47

Closed
wants to merge 23 commits into from
Closed

Updates to Install and Rules pages #47

wants to merge 23 commits into from

Conversation

JasonHoku
Copy link
Contributor

Compiled from edenxi.com and discord from 5/22/20.

Some formatting changes and info directly added to Install & Rules and some added to config for now

@JasonHoku
Copy link
Contributor Author

Ino said I was on an older base so the conflicts should be good in this PR instead

@JasonHoku
Copy link
Contributor Author

Oh wait, maybe I just am not running prettier on this before committing? @_@;
Ill get this figured out soon I promise lol

JasonHoku added 2 commits May 23, 2020 22:03
@JasonHoku
main pr: updates to install and rules
14a9893 
also ran npm update for vulnerabilities upgrade typescript and node
@JasonHoku
applied npm run format *
e815f00
@JasonHoku
Copy link
Contributor Author

Yesss, I did it!!! xD

@InoUno
Copy link
Collaborator

InoUno commented May 24, 2020

Hmm, looks like some formatting has been applied to all files and not just JS/TS ones, which is what the format command should only be targeting.

Did you just run npm run format, or did you also try some other things that might have caused JSON/CSS files to be formatted as well?
We probably want to format those too eventually (better to do in separate PR), I'm just curious if there might be some setup differences that makes the format command hit JSON/CSS files as well for you.

@JasonHoku
Copy link
Contributor Author

Hmm.. I believe I ran update, upgrade and run-script format as well in this PR, I can rebase and re-pr without running those.

@InoUno
Copy link
Collaborator

InoUno commented May 25, 2020

Running npm update or npm upgrade should only really be done if there's a specific need to update any/all packages. They shouldn't be needed for regular PRs. But it's fine for now, we can keep the updated version in this PR.

Doing those things shouldn't have formatted all the rest of the files though. Gonna try to figure it out with you via discord.

@InoUno
Copy link
Collaborator

InoUno commented May 25, 2020 •
edited
Loading

Looks good now regarding the files changed (could do without the package updates, but that's fine for now).

I'll let @slassen review the text/content changes.

@InoUno
Copy link
Collaborator

InoUno commented May 27, 2020 •
edited
Loading

I took the liberty of restoring the package files, so they won't cause a lot of conflicts with the other PRs.

@slassen
Copy link
Collaborator

slassen commented May 28, 2020

I like the last updated timestamp on terms and conditions. Maybe we can copy this for each section?

I would rather have some sort of table for the allowed/disallowed plugins instead of the way it's set up in this PR.

I would rather have something in italics instead of another numbered bullet point with a ^ signaling that it relates to the above point.

Please use the previous elements and styling on Discord rules.

For the "Punishments" section please use a semantic table.

@bkesecker
Copy link
Contributor

fixes #5

bkesecker
bkesecker suggested changes May 28, 2020
View reviewed changes
client/src/components/install.jsx Outdated
@@ -8,7 +8,7 @@ function install(props) {
const { info } = props;

return (
<Container>
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably invalid XML. Maybe just comment out? Can clients override style "display: none"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Man.. I had addon interference in my VSC... really messed me up for a minute there lol but then I discovered how to use Prettier, this next PR should fix any formatting issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can clients override style "display: none"?

I don't think so, when node.js compiles everything it removes all comment lines in the final client .js build files to make it very efficient for production.

JasonHoku added 3 commits May 28, 2020 09:47
@JasonHoku
[x]I like the last updated timestamp on terms and conditions. Maybe w…
caa550d 
…e can copy this for each section?

[x * This might take some work, tidied a little for now]I would rather have some sort of table for the allowed/disallowed plugins instead of the way it's set up in this PR.

[x]I would rather have something in italics instead of another numbered bullet point with a ^ signaling that it relates to the above point.

[x]Please use the previous elements and styling on Discord rules.

[x]For the "Punishments" section please use a semantic table.
@JasonHoku
ran prettier :P
5fe4333
@JasonHoku
Merge remote-tracking branch 'origin/disable-login' into PR-Install-&…
370a6cc 
…-Rules-info-updates
@bkesecker
Copy link
Contributor

ran prettier

Did this change all the double quotes to single quotes?

@JasonHoku
Copy link
Contributor Author

ran prettier

Did this change all the double quotes to single quotes?

Maybe my prettier config isn't setup right?

@JasonHoku
Copy link
Contributor Author

ran prettier

Did this change all the double quotes to single quotes?

Maybe my prettier config isn't setup right?

Looking into it.

@bkesecker
Copy link
Contributor

Looks like the other way around. Tough to review with so many changes.

@JasonHoku
Copy link
Contributor Author

image

Hmm.. I guess that it was a default setting, I may have reset some prettier options in VSC ?

I did find a fix for the single quote replace and I will undo that change now.

@bkesecker
Copy link
Contributor

I did find a fix for the single quote replace and I will undo that change now.

Thank you. Will need to merge with develop to resolve conflicts.

@JasonHoku
Copy link
Contributor Author

I was able to choose to force either double quote or single quotes with prettier but I couldn't get it to run without interfering..
pretty sure this article is whats wrong with end right now, but Ill have to pick back up on it later tonight for now.

@JasonHoku
Copy link
Contributor Author

I realllly hope I did this right >< I possibly should have changed my merge order slightly but I resolved the conflicts manually which looked like the login-fix pr branch didn't get applied to this branch after some of my reverts and merges, but we should be ready to roll now. Thanks everyone for the patience while I get up and running with git :P

@slassen
Copy link
Collaborator

slassen commented May 29, 2020

No problem. I’m sure you’re almost there! I think it would probably be best if you copied just the files of your changes you wanted to make to your desktop then switch to the develop branch and make a new branch/PR. Additionally, please make sure to review and address the points in my previous comment.

@JasonHoku
Copy link
Contributor Author

No problem. I’m sure you’re almost there! I think it would probably be best if you copied just the files of your changes you wanted to make to your desktop then switch to the develop branch and make a new branch/PR. Additionally, please make sure to review and address the points in my previous comment.

I am so into that idea. This one has gotten a bit messy ^^;

@JasonHoku JasonHoku closed this May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkesecker bkesecker bkesecker requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JasonHoku @InoUno @slassen @bkesecker @danny-vvv