dwmkerr / crosswords-js

Tiny, lightweight crossword control for the web.
https://dwmkerr.github.io/crosswords-js/
MIT License
71 stars 27 forks source link

Omnibus of new features and fixes #42

Closed pvspain closed 8 months ago

pvspain commented 9 months ago

Apologies for the size!

I think the only outstanding TODO is to improve accessibility. Changes in reverse chronological order:

Fix: Relocate keybinding to precede gridView construction Fix: Revert cell id to '(x,y)' Relabel API buttons in demo: Test -> Check Add documentation and samples for .ipuz puzzle format. Fix: setCellAnswer() - don't clobber existing Add documentation of puzzle format .puz Fix: Improve demo load time Package: Add script to bootstrap project and tools from a freshly cloned git repository in a Linux environment. Add test sample and doc for grid resizing. Feature: Markdown support in clueText. Refactor controller construction. Improve documentation. Add error handling for CrosswordDefinition conversion. Refined crossword domain documentation Fix: Change usage of event.keyCode to event.key Event.keyCode is deprecated! https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode Doc: Miscellaneous small tweaks Feature: Customisable keyboard shortcuts and actions. Fix: conditionally set currentCell in event:focus Doc: Improve samples section (crossword-domain.md) Add default keybindings Feature: (Re)set crossword file on live controller. Package: Add project bootstrap script for Linux. Simplify controller property names. Refactor: Restructure source: separate files for gridview and cluesview Refactor: Extract methods and helpers from Controller to reduce complexity. Refactor: Extract functions to reduce complexity Package: Add package script "update". Update nvm, npm, node LTS and packages to latest versions. Sort package scripts. Fix: definition for clueModel.lengthText Revise clue model regexes to allow whitespace. Extend testing. Extend doc and modify for changes. Doc: Refine docs for clueModel and crosswordDefinition Improve crossword validation Feature: Dynamically load new puzzle Feature: Expose controller.setCell() to programmatically set grid cell text Doc: tweak description of TAB keystroke. Refactor: Refactor CellMap to optimise performance modified: src/cell-map.mjs modified: src/crossword-controller.mjs Refactor: Extract memoized functions Refactor: Extract method #delayPublish Remove method context from assert() calls - given in stack dump. Fix: Processing of (focus,click) event sequence Fix: Add moveBackCell() - direction of movement now based on current clue. Refactor: Renaming: anchor:non-anchor -> head:tail Doc: Improve domain concepts documentation. Change code to match. Fix: Handle absent clue.solution Feature: Add 'incomplete' events for clue and crossword testing.

pvspain commented 9 months ago

Hi Misha @mmkal,

I have included your changes from PR38, but they have been relocated with refactoring of src/crossword-controller.mjs to reduce complexity. The CSS vars are now set inside newCrosswordGridView() in src/crossword-gridview.mjs.

pvspain commented 8 months ago

This looks great from eyeballing it! A small comment about yaml being in dependencies so far.

I can help test this, but I'll need the css variables change to go in first or be included in this branch - I'm relying on that for styling in my app.

If you want to test these mods, I've written a gist on processing pull requests locally. This method enables you to evaluate changes without interrupting your ongoing work. I use the process regularly before submitting PRs to ensure they go in smoothly.

dwmkerr commented 8 months ago

This looks great - I'd say merge and fix forward for any issues, as this is such a large one. Want me to hold off until we've got more clarity on the YAML side of things?

pvspain commented 8 months ago

I vote we merge now and refactor later. Let's keep the velocity up. We don't want a backlog of PR's, as it makes merging later PRs more difficult.

To that end, how about a policy of only submitting PRs that are ready to merge? The PR must merge smoothly into dmwkerr/main, as @dwmkerr has requested previously. The onus is on the submitter to ensure that by test-merging offline. To assist that, I've written a gist to work through the PR locally, which I use for every PR, and also for merging changes back upstream. There are several checkpoints to test and inspect the changes.

Lets have another channel for discussing the roadmap and technology changes? We don't want wasted effort in PR's - nobody's getting paid here! @dwmkerr's original TODO is complete with this PR, apart from improving accessibility; tabbing out of the gridview, for example, is impossible with the default keybindings. I imagine there's prior art for that problem. Do you want to set up another channel Dave? @dwmkerr

I'm happy with segregation into different modules, a multi-package mono-repo - just no more complicated than necessary right now. We can always refactor. Knock yourself out @mmkal! I have no issue with keeping this core repo dedicated to compiling crossword definitions (JS Objects) and rendering the different views - grid, clues. With the proviso that our hello world remains simple, as does adding YAML support. I think we're all agreed it's a better format for human editing.

On a side note, I've switched to js-yaml from yaml on an upstream branch and its painless. The example yaml puzzles all convert happily. I see the major task with extracting YAML is rewriting the docs, as there are a lot of links and references to YAML now. It will be easier if we have another repo set up before all that editing begins.

mmkal commented 8 months ago

That's ok with me - maybe as a simpler follow-up to this I could submit a pull request which switches js-yaml to be an optional peer dependency instead. I agree that end users should be encouraged to write yaml over json, I'm just nit-picking implementation details from the perspective of a developer-user rather than a webapp-user.

(And of course, I am just a user of this package giving feedback, so things shouldn't be held up on my account!)

dwmkerr commented 8 months ago

Love it - I'm going to merge

dwmkerr commented 8 months ago

This is amazing work - @pvspain and @mmkal should we start some kind of design doc we can work on offline on how we want to structure things going forwards? I like the idea of an org for this, and maybe modules like:

dwmkerr commented 8 months ago

We could set a timeline / set or priorities, modularise a bit and then divide and conquer based on interests, what do you think? Could then have code owners per module, e.g. we all own the format/foundational stuff, but divide and conquer on samples?

dwmkerr commented 8 months ago

It also democratises the ownership a bit and lets you both move quickly, with my workload I don't want to be a blocker, but would love to give input (and maybe own the dorky commandline version)