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

Characters #78

Merged
merged 7 commits into from
Dec 4, 2024
Merged

Characters #78

merged 7 commits into from
Dec 4, 2024

Conversation

PilotKayer
Copy link

@PilotKayer PilotKayer commented Oct 18, 2024

Type
What type of pull request is this? (e.g., Bug fix, Feature, Refactor, etc.)

  • Bug fix
  • Feature
  • Refactor
  • Other (please describe):

Description
A clear and concise description of the changes you are making. What does this PR implement or fix?

CSS and HTML updates on both Adversary and Character sheets to better align with what was proposed by MangoVFTT. It is going for a more clean look with less visual clutter. See screenshot attached.

Related Issue
This PR must be linked to an existing issue. Please specify the related issue number and whether this PR fully or partially resolves it. Use the format Closes #<issue-number> or Partially addresses #<issue-number>.

Partially closes #44 and #47 . Closes #71 .

How Has This Been Tested?
Describe how you tested the changes and how others can replicate the testing steps.

Screenshots (if applicable)
Add screenshots or GIFs that help illustrate the changes, especially if they affect the UI or user-facing aspects of the system.
image

Checklist:

  • I have commented on my code, particularly in hard-to-understand areas.
  • My changes do not introduce any new warnings or errors.
  • My PR does not contain any copyrighted works that I do not have permission to use.
  • I have tested my changes on Foundry VTT version: Version 12 Stable - Build 331.

Additional context
Add any other context or information here that would be useful for reviewers.
Colours setting have been updated to work with variables. This means that it should fully support themes and therefore should be able to close #71.
I have made changes in the "default" colours and look from what was originally proposed to avoid visual clutter. This sheets are game pieces that need to be clean and clear to understand. I agree that making them pretty was necessary to "bling" the system. However, the proposed sheets had too much details and too much use of the accent colour. I scaled down the use of special SVGs and the accent colour, and instead opted for a clean look to make it a more clear game piece rather than something which simply looks good.

src/style.scss Outdated Show resolved Hide resolved
src/style.scss Outdated Show resolved Hide resolved
@TheFlyingPig
Copy link

TheFlyingPig commented Oct 18, 2024

This is some great work! I think MangoFVTT is right, we should stick as close as possible to their proposed design, and then for changes we should discuss and debate them in Discord or on an issue here. We should also design for the lowest performing systems, so any transparency should be option based in my opinion, though I do like it.

Updated borders and colors to better match the original Character sheet. Adversary sheet is currently completely broken.
@PilotKayer
Copy link
Author

Draft 2 release for the Character sheet.

This draft has been built to better represent the original design by @MangoFVTT .

To Note:

  1. Adversary sheet is currently broken in this build
  2. I have left in the gradients - please let me know if I should leave it in or remove it until we figure out how to add the suggested "HD pack"
  3. This has not been implemented for "light mode", and is, therefore, broken.

Screenshots:
image
image
image
image
image

@PilotKayer PilotKayer changed the base branch from release-0.1.0 to release-0.3.0 November 23, 2024 14:16
Moved the last few pixels and changed the color variables to work with both default and stormlight theme
@PilotKayer PilotKayer marked this pull request as ready for review November 27, 2024 17:18
Copy link
Collaborator

@zithith zithith left a comment

Choose a reason for hiding this comment

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

Overall comments:
This is great and I think we want to get it in soon, but some comments for potential future passes:

  • There's a lot of hard-coded paths in the html templates, ones that I suspect are the same?... can we find a way to inject these dynamically at least, so that there is less maintenance overhead. and to potentially also optimise the final markup too.
  • These CSS files are getting HUGE. I know it wasn't part of this issue, but we probably want a ticket to spend time really thinking about how we can reduce duplication of definitions and optimise this, both for the SCSS in the repo and the size of the outputted native CSS

text-shadow: 0 0 0.5rem black;
box-shadow: 0 0 0.5rem black;

.border {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this border block is exactly the same as the one above (or very similar to it and/or other declarations for borders) could we maybe make a SCSS mixin?

Copy link
Collaborator

@MangoFVTT MangoFVTT left a comment

Choose a reason for hiding this comment

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

I agree with Zitith, I'm worried about all the hardcoded paths in the templates because it means we can't easily change SVGs if we want to. It's gonna be hell to maintain if we ever need to change layouts and sizes of borders. Other than that though this is fine. I'll do a clean-up pass on the css when I implement colours for light themes

@zithith zithith merged commit 809e3ac into the-metalworks:release-0.3.0 Dec 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants