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

Minor fixes and updates #197

Merged
merged 8 commits into from
Oct 24, 2021
Merged

Minor fixes and updates #197

merged 8 commits into from
Oct 24, 2021

Conversation

corowne
Copy link
Owner

@corowne corowne commented Oct 17, 2021

Reviewing the code body in general, putting in some minor changes that are mostly small fixes and stylistic changes for consistency (coding/visually/English; will be...quite...pedantic). Still WIP.

  • Fix SalesController index
  • Indentation
  • Remove consecutive <br>s in favour of margins
  • A few visual changes

Todo:

  • Pull large chunks of PHP code out of the views (primarily noticed in comments)
  • Add a top_comment_id to all comments (and an update script to process existing comments which will hopefully not kill anyone's site) to remove recursive querying
  • Pull out comment modals so that the number of modals on the page are reduced
  • Reduce duplicated code (just noticed that _perma_comments.blade.php is almost identical to _comment.blade.php)
  • Rename controller functions to match the getSomething/postSomething pattern?
  • Script to process stacking items to compress existing inventory
  • Fix the active sidebar item on the /user/{username}/favorites/own-characters route
  • Log page visuals ("table" styling)

Notes on consistency:

  • Variables should be camelCase unless it's a model property
  • Technically there's no issue with leaving them out, but I used trailing slashes for self-closing tags in the original HTML code so adding them for consistency (<hr />, <br />, <img src="" /> rather than <hr>, <br>, <img src="">)
  • Can let single <br> slide, but use margins in place of <br><br> (the Bootstrap mt-3, mb-3 classes can be used for this)
  • Indentation is 4 spaces. The {!! Form::open() !!} and {!! Form::close() !!} tags should be counted as open/close tags.
  • {!! Form::open() !!} and {!! Form::close() !!} preferred to <form></form> tags; the former also automatically includes the CSRF token so it's not needed.
  • Avoid using the style attribute in HTML tags (this will override custom CSS), make it classes as far as possible (would say that elements that use a lot of Bootstrap classes should also be converted to CSS in lorekeeper.css, as the Bootstrap classes have !important on them and may be annoying for users to customise)
  • Use full-stops for sentences.

There are some things I'd like to rework in comments (taking PHP code out of the views, reducing the number of modals, removing recursive querying) but I'll wait till Newt's PR has been integrated to reduce conflicts.
@itinerare itinerare requested review from itinerare and Draginraptor and removed request for Draginraptor and itinerare October 17, 2021 12:34
# Conflicts:
#	resources/views/comments/_comment.blade.php
#	resources/views/user/profile.blade.php
…er. Users without the maintenance access power will get redirected to the home page.
@itinerare itinerare added the enhancement New feature or request label Oct 23, 2021
@corowne corowne marked this pull request as ready for review October 23, 2021 18:40
@corowne
Copy link
Owner Author

corowne commented Oct 23, 2021

I think this is good enough, the other changes would take a while longer to do and I don't want to push any more functionality changes now!

@itinerare itinerare added the needs review Pull requests that are pending community review label Oct 23, 2021
@itinerare itinerare added reviewed Pull requests that have received community review and are pending merge and removed needs review Pull requests that are pending community review labels Oct 24, 2021
@itinerare itinerare merged commit 76df6b9 into develop Oct 24, 2021
@itinerare itinerare deleted the feature/minor-2.0.0-fixes branch October 24, 2021 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request reviewed Pull requests that have received community review and are pending merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants