dwmkerr / crosswords-js

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

Develop #34

Closed pvspain closed 10 months ago

pvspain commented 10 months ago

Hi Dave, long time no PR! I hope you're keeping well?

Sorry to dump a truckload on you. I was trying to submit a cohesive and complete change, but it just kept growing. I've rebased all my commits to reduce the number of changes to you need to review and QA, but there is still a lot!

Here are the big ticket items:

pvspain commented 10 months ago

Hi Dave, If you'd rather deal with the PR locally in your own IDE rather than GitHub, I've written a gist with step by step instructions:

dwmkerr commented 10 months ago

Let's just get it in! Merge it all and fix forwards if needed?

dwmkerr commented 10 months ago

Thanks for all of this!

pvspain commented 10 months ago

Hi Dave,

I'm happy to merge in the PR #34 changes, but I don't have write access to the repo. I've merged the changes to a local version but can't push to the remote.

You've made me a contributor but not a collaborator methinks - I can't see the Settings menu.

https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-access-to-your-personal-repositories/inviting-collaborators-to-a-personal-repository#inviting-a-collaborator-to-a-personal-repository

Cheers, Paul

mmkal commented 10 months ago

Hi @pvspain and @dwmkerr, just a note on this. I tried it out by installing c074f80024ee162c1f811ec9c30740024420a435 as an npm module via github, something like this in package.json

"dependencies": {
  "crosswords-js": "github:pvspain/crosswords-js#c074f80024ee162c1f811ec9c30740024420a435"
}

I was able to get it to work but only after also installing less, and running lessc node_modules/crosswords-js/style/crosswords.less src/styles/crosswords-js-compiled.css and then using that compiled css file instead of the shipped one, which didn't seem to style things correctly.

I'm also doing import { CrosswordsJS } from 'crosswords-js/src/index.mjs' rather than using the dist folder, so that may be why. But it does seem that dist/crosswords.css is out of date, and since it's checked in, that should probably be addressed. I didn't start digging into the webpack config to see if something should be changed to make it spit out the right css, because npm install less worked first time.

mmkal commented 10 months ago

Also, I've started writing some typescript declarations for the core methods, please let me know if you'd be interested in a pull request adding them to the main repo.

pvspain commented 10 months ago

Hi @mmkal, we're merging a lot of changes and there's a bit of a disconnect in the main branch. I don't think the angular sample has been working for some time. We're using Vite for bundling in the incoming code for the sample in dev and Webpack continues in the main branch. Less has been added to the package in the incoming code, and the Vite prod artifacts are currently appearing in dev/dist on running npm run dev:build in the incoming branch.

The typescript wrapper would be great. I'd hold off for now till the merge is complete.

Thanks for your patience and interest!

pvspain commented 10 months ago

Hi Dave,

I'm happy to merge in the PR #34 changes, but I don't have write access to the repo: https://github.com/dwmkerr/crosswords-js I've merged the changes to a local version but can't push to the remote.

You've made me a contributor but not a collaborator methinks - I can't see the Settings menu. https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-access-to-your-personal-repositories/inviting-collaborators-to-a-personal-repository#inviting-a-collaborator-to-a-personal-repository

Cheers, Paul

On Wed, 16 Aug 2023 at 04:33, Dave Kerr @.***> wrote:

Thanks for all of this!

— Reply to this email directly, view it on GitHub https://github.com/dwmkerr/crosswords-js/pull/34#issuecomment-1679411596, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFO4S4WNXCRI5UR53IMJ3TXVO6JFANCNFSM6AAAAAA3LCOXDI . You are receiving this because you authored the thread.Message ID: @.***>

pvspain commented 10 months ago

Changes moved to pvspain/main and new pull request issued from there.