Conversation
|
I'll be taking a few screenshots for folks who want to give feedback without pulling it locally |
Weapons in containers appear bugged: can't be clicked on or dragged out of the container. Drag behavior has some issues we'll address in a separate comment
New actor. I like the little hover animation
|
| "browser": true | ||
| }, | ||
| "ignorePatterns": ["**/lang/**/*.json", "**/*.d.ts"], | ||
| "ignorePatterns": ["rollup.config.mjs", "**/lang/**/*.json", "**/*.d.ts"], |
There was a problem hiding this comment.
rollup config is removed from linting? any particular reason?
There was a problem hiding this comment.
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? |
| "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.
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.
I forget what our conversation on this was. Why are these included as dependencies when Foundry includes fontawesome?
| import path from 'path' | ||
| import { defineConfig } from "rollup"; | ||
|
|
||
| // shared rollup plugins |
There was a problem hiding this comment.
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.
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.
We only get to use Foundry's font scaling settings if we use Foundry's own font size variables
| "browser": true | ||
| }, | ||
| "ignorePatterns": ["**/lang/**/*.json", "**/*.d.ts"], | ||
| "ignorePatterns": ["rollup.config.mjs", "**/lang/**/*.json", "**/*.d.ts"], |
There was a problem hiding this comment.
Sorry that I forgot how to contain everything in a single review for a couple of these!
anthonyronda
left a comment
anthonyronda
left a comment
There was a problem hiding this comment.
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.
it's missing the (admittedly hard to make extensible) tick marks for each of the encumbrance step-down points
There was a problem hiding this comment.
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.
I don't have a lot of imagination for why this has to be expandable. I'll have to go back to the original design to see what that looked like
|
this is frustrating. 5-6 of my review comments got lost :( |
2 participants
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.
