Skip to content

Modal v1#294

Merged
mjbp merged 7 commits intomasterfrom
feature/mick/187-modal
Mar 3, 2025
Merged

Modal v1#294
mjbp merged 7 commits intomasterfrom
feature/mick/187-modal

Conversation

@mjbp
Copy link
Copy Markdown
Collaborator

@mjbp mjbp commented Feb 20, 2025

Resolves #283

  • sweep of codebase to elimate dead code, run linting, fix any non-breaking niggles or annoyances
  • sweep of docs
  • clean-up example
  • sweep of tests
  • update store to standardise
  • publish src folder to npm
  • add custom open and close events

@mjbp mjbp linked an issue Feb 20, 2025 that may be closed by this pull request
@mjbp mjbp changed the title Feature/mick/187 modal Modal v1 Feb 20, 2025
Copy link
Copy Markdown

@catvine-stormid catvine-stormid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few little (potential) typos :)


## Options
Options can be set during initialising in an Object passed as the second argument to the modal function, e.g. `modal('.js-modal', { startOpen: true })`, or as data-attributes on the modal element (the element passed to the modal function), e.g. `data-start-open="true"`
Options can be set during initialising in an Object passed as the second argument to the modal function, e.g. `modal('.js-modal', { startOpen: true })`, or as data-attributes on the modal element (the element passed to the initiaisation function), e.g. `data-start-open="true"`
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small typo - *initialisation.

/*
* Sets aria attributes and adds eventListener on each tab
* Partially applied function that returns a function
* Sets aria attributes and adds eventListener on each nodal toggle
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "nodal toggle" meant to be "modal toggle"?

Copy link
Copy Markdown
Contributor

@sarah-richards-stormid sarah-richards-stormid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing additional to Cat's comments!

@mjbp mjbp merged commit 83e054c into master Mar 3, 2025
1 check passed
@mjbp mjbp deleted the feature/mick/187-modal branch March 3, 2025 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modal > v1

3 participants