-
Notifications
You must be signed in to change notification settings - Fork 57
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
feat: Character Sheet v2 #479
base: main
Are you sure you want to change the base?
Conversation
I'll be taking a few screenshots for folks who want to give feedback without pulling it locally |
@@ -16,7 +16,7 @@ | |||
"env": { | |||
"browser": true | |||
}, | |||
"ignorePatterns": ["**/lang/**/*.json", "**/*.d.ts"], | |||
"ignorePatterns": ["rollup.config.mjs", "**/lang/**/*.json", "**/*.d.ts"], |
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.
rollup config is removed from linting? any particular reason?
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.
Sorry that I forgot how to contain everything in a single review for a couple of these!
shield.svg doesn't appear to be in use shield2.svg looks like the aspect ratio is a bit messed up? |
@@ -51,22 +54,30 @@ | |||
"eslint-plugin-promise": "^6.1.1", | |||
"eslint-plugin-scanjs-rules": "^0.2.1", | |||
"eslint-plugin-security": "^1.5.0", | |||
"eslint-plugin-simple-import-sort": "^8.0.0", |
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.
yeah this one was really being a PITA
I could be wrong but I think the Tweaks menu functionality should also be written somewhere as TODO items |
"@fortawesome/fontawesome-free": "^6.4.0", | ||
"@fortawesome/fontawesome-svg-core": "^6.4.0", | ||
"@fortawesome/free-solid-svg-icons": "^6.4.0" |
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.
I forget what our conversation on this was. Why are these included as dependencies when Foundry includes fontawesome?
import { defineConfig } from "rollup"; | ||
|
||
// shared rollup plugins |
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.
what is meant by shared vs system rollup plugins? UFT vs OSE?
if (!this.hasAttribute("readonly") && !this.hasAttribute("disabled")) | ||
this.shadowRoot | ||
?.querySelector("label") | ||
?.addEventListener("click", (e) => { | ||
const evt = new Event("roll") as Event & {metaKey: boolean, ctrlKey: boolean}; |
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.
what is the reasoning here? Why do we check if the ability score is editable before allowing rolls on it? When is it disabled?
font-size: var(--spacing-full); /* TODO: variables! ems! */ | ||
line-height: 38px !important; |
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.
We only get to use Foundry's font scaling settings if we use Foundry's own font size variables
@@ -16,7 +16,7 @@ | |||
"env": { | |||
"browser": true | |||
}, | |||
"ignorePatterns": ["**/lang/**/*.json", "**/*.d.ts"], | |||
"ignorePatterns": ["rollup.config.mjs", "**/lang/**/*.json", "**/*.d.ts"], |
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.
Sorry that I forgot how to contain everything in a single review for a couple of these!
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.
Less of a request for changes, more of a request for dialog :)
get #value() { | ||
const value = parseInt(this.getAttribute("value") || ''); | ||
if (isNaN(value)) return 0; | ||
return value; | ||
} | ||
|
||
get #max() { | ||
const max = parseInt(this.getAttribute("max") || ''); | ||
if (isNaN(max)) return 100; | ||
return max; | ||
} | ||
|
||
get #progress() { | ||
return (this.#value / this.#max * 100) || 0; | ||
} |
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.
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.
I like how it looks though
<uft-expandable-section type="ability" can-create aria-expanded> | ||
<h2 slot="heading">{{localize "OSE.category.abilities"}}</h2> |
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 frustrating. 5-6 of my review comments got lost :( |
Intent
This changeset implements Phase 1 of the new character sheet, while attempting to maintain the existing character sheet.
Wait, phase one? We never discussed phases!
In the interest of getting this sheet out this year, I'm opting to put it up for review at feature parity (plus a little extra) with the current sheet. In my opinion, adding things like spell sources, ability categories, and so on are too disruptive to the existing character sheet; I'd prefer to hold off on them until we can safely end-of-life the old sheet.
What's missing?
Where did I veer "off road"?
Anything else of note?
Beyond that... This isn't 100% ready to go in, but it can be -- with your help. There's a lot here. I've been staring at it for a long time, and I'm likely missing stuff. Pull the sheet down, and really put it through the ringer.