-
Notifications
You must be signed in to change notification settings - Fork 1
Refactor of globals and State #2
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
03205c8
a879d41
5cda4e2
c004456
b207c45
15aac2d
ec72c46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| *.md | ||
| *.yml | ||
| coverage | ||
| Makefile | ||
| .prettierignore | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,9 +127,8 @@ | |
| } from "./lib/helpers.js"; | ||
|
|
||
| let { urlData } = $props(); | ||
| let globals = $state( | ||
| load(urlData) ?? new State([new Sheet("Sheet 1", 10, 10)]), | ||
| ); | ||
| let globals; | ||
| globals = load(urlData) ?? new State([new Sheet("Sheet 1", 10, 10)]); | ||
|
||
| let table = $state(); | ||
| let startHeight = $state(0); | ||
| let scrollArea = $state(); | ||
|
|
@@ -162,7 +161,7 @@ | |
| return undefined; | ||
| } | ||
| } | ||
| return State.load(data); | ||
| return State.loadNew(data); | ||
| } | ||
|
|
||
| let dontSave = $state(false); | ||
|
|
@@ -309,15 +308,7 @@ | |
| return; | ||
| } | ||
| dontSave = true; | ||
| globals = Object.assign(State.load(e.state), { | ||
| currentSheetIndex: globals.currentSheetIndex, | ||
| mode: globals.mode, | ||
| helpOpen: globals.helpOpen, | ||
| editorOpen: globals.editorOpen, | ||
| imageOpen: globals.imageOpen, | ||
| elements: globals.elements, | ||
| pasteBuffer: globals.pasteBuffer, | ||
| }); | ||
| globals.load(e.state); | ||
| }} | ||
| bind:innerHeight | ||
| /> | ||
|
|
@@ -326,12 +317,12 @@ | |
| {#if navigator.maxTouchPoints <= 1} | ||
| <!-- Else float the bar above the virtual keyboard --> | ||
| <div> | ||
| <FormulaBar bind:globals bind:editor={globals.elements.formulaBar} /> | ||
| <FormulaBar {globals} bind:editor={globals.elements.formulaBar} /> | ||
| </div> | ||
| {/if} | ||
|
|
||
| <div class="tabs"> | ||
| <Tabs bind:globals bind:value={globals.currentSheetIndex} /> | ||
| <Tabs {globals} bind:value={globals.currentSheetIndex} /> | ||
| </div> | ||
|
|
||
| <div | ||
|
|
@@ -350,7 +341,7 @@ | |
| Set --width and --height default values because if Table is in a Dialog, it | ||
| will inherit the width and height of the dialog for table cells. | ||
| --> | ||
| <Table bind:globals bind:table --width="auto" --height="auto" /> | ||
| <Table {globals} bind:table --width="auto" --height="auto" /> | ||
| </div> | ||
|
|
||
| <Dialog | ||
|
|
@@ -387,7 +378,7 @@ | |
| <p>Error {err}</p> | ||
| {/await} | ||
| </Details> | ||
| <SaveLoad bind:globals {imageData} /> | ||
| <SaveLoad {globals} {imageData} /> | ||
| </div> | ||
| </Dialog> | ||
|
|
||
|
|
@@ -450,7 +441,7 @@ | |
| </Details> | ||
| <Details> | ||
| {#snippet summary()}Settings{/snippet} | ||
| <Settings bind:globals /> | ||
| <Settings {globals} /> | ||
| </Details> | ||
| </div> | ||
| </Dialog> | ||
|
|
@@ -489,7 +480,7 @@ | |
| {#if navigator.maxTouchPoints > 1} | ||
| <!-- Must set top instead of bottom for correct placement on iOS --> | ||
| <div class="keyboardbar" style:top="calc({visualBottom}px - 2.5em * 2)"> | ||
| <FormulaBar bind:globals bind:editor={globals.elements.formulaBar} /> | ||
| <FormulaBar {globals} bind:editor={globals.elements.formulaBar} /> | ||
| {#if showInputButtons} | ||
| <div class="buttonbar" style:z-index={nextZIndex()}> | ||
| {@render insertTextButton("=")} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,29 +56,30 @@ functions.crypto = async (ticker) => { | |
| }); | ||
| forceSave = $state(0); | ||
|
|
||
| static load(data) { | ||
| let result = new State( | ||
| data.sheets.map((sheet) => { | ||
| constructor(sheets, formulaCode) { | ||
| this.load({ sheets, formulaCode }); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this happens to work, but isn't quite correct. We can either change the constructor, or change what we pass to it. (I lean towards the latter.) Right now, the constructor is called on I think the best fix is to never pass fully initialized
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fun fact: TypeScript would probably have caught this. Gonna have to set that up at some point...
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah good catch, thank you.
Yeah probably. Also consider jsdoc if you don't want to deal with the transpiling, but then again svelte has a compilation step anyway |
||
| } | ||
|
|
||
| static loadNew(data) { | ||
| return new State(data.sheets, data.formulaCode); | ||
| } | ||
|
|
||
| load({ sheets, formulaCode }) { | ||
| Object.assign(this, { | ||
| sheets: sheets.map((sheet) => { | ||
| let s = new Sheet( | ||
| sheet.name, | ||
| sheet.heights.length, | ||
| sheet.widths.length, | ||
| (i, j) => sheet.cells[i][j].formula, | ||
| (i, j) => sheet.cells[i][j].value, | ||
| ); | ||
| s.globals = this; | ||
| s.widths = sheet.widths; | ||
| s.heights = sheet.heights; | ||
| return s; | ||
| }), | ||
| data.formulaCode, | ||
| ); | ||
| return result; | ||
| } | ||
|
|
||
| constructor(sheets, formulaCode) { | ||
| this.sheets = sheets; | ||
| this.sheets.forEach((sheet) => { | ||
| sheet.globals = this; | ||
| formulaCode: formulaCode, | ||
| }); | ||
| if (formulaCode != null) { | ||
| this.formulaCode = formulaCode; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,9 @@ | ||
| import { Sheet, State } from "../src/classes.svelte.js"; | ||
| import { State } from "../src/classes.svelte.js"; | ||
| import { test, expect, beforeEach } from "vitest"; | ||
| import { evalCode, functions } from "../src/formula-functions.svelte.js"; | ||
|
|
||
| function createSheet(cells, formulaCode = "") { | ||
| return State.load({ | ||
| return State.loadNew({ | ||
| sheets: [ | ||
| { | ||
| name: "Sheet 1", | ||
|
|
@@ -690,3 +690,25 @@ test("Binary literals", async () => { | |
| [8, -8, -9, 7, 9], | ||
| ]); | ||
| }); | ||
|
|
||
| test("State.load preserves elements", async () => { | ||
| const state = createSheet([["1", "2", "3"]]); | ||
| await expectSheet(state.currentSheet, [[1, 2, 3]]); | ||
| state.elements.formulaBar = document.createElement("textarea"); | ||
| state.elements.formulaBar.setAttribute("data-testid", "shouldPreserve"); | ||
|
|
||
| const savedState = createSheet([["4", "5", "6"]]); | ||
| await expectSheet(savedState.currentSheet, [[4, 5, 6]]); | ||
|
|
||
| const testFormulaCode = "functions.testFormulaCode = () => {}"; | ||
| state.load({ | ||
| sheets: savedState.sheets, | ||
| formulaCode: testFormulaCode, | ||
| }); | ||
|
|
||
| await expectSheet(state.currentSheet, [[4, 5, 6]]); | ||
| expect(state.formulaCode).toBe(testFormulaCode); | ||
| expect(state.elements.formulaBar.getAttribute("data-testid")).toBe( | ||
| "shouldPreserve", | ||
| ); | ||
| }); | ||
|
Comment on lines
+708
to
+728
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my view, this is really two different tests: one tests that the bug you originally reported is fixed (i.e., the formula bar is not broken by loading a sheet), while the other tests that What might make sense instead is to add a much more comprehensive |
||
Uh oh!
There was an error while loading. Please reload this page.