Skip to content
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

Allow filters on a Toolbar to be controlled externally #1231

Merged
merged 1 commit into from
Jun 21, 2022

Conversation

danwatford
Copy link
Contributor

Q A
Bug fix? [ ]
New feature? [X]
New sample? [ ]
Related issues? fixes #1222

What's in this Pull Request?

Additions to properties of Toolbar and Toolbar filter to permit external control of the selected filters displayed by the Toolbar.

Non-breaking signature change to Toolbar's onSelectedFiltersChange callback, allowing function implementations to return void rather than forcing implementations to return the set of selected filters that the Toolbar should display.

Highlight difference between controlled and uncontrolled Toolbars in the ControlsTest webpart. Users can operate buttons to toggle selected filters and observe that the Toolbar responds in a controlled manner.

Documentation changed to describe controlled vs uncontrolled use of the Toolbar.

@danwatford
Copy link
Contributor Author

I ran into some problems with tests and pre-commit hooks for this PR.

Attempts to run units tests against the dev branch (i.e. without this PRs changes would fail). After applying this PR's changes the same failure were observed so I reasoned that this PR had not broken any tests.

I had to commit with the --no-verify option to prevent the husky pre-commit hook from running. This hook appears to be related to generating changelog mark-up that wouldn't have been included in the commit anyway. The following error occurred when running npm run-script changelog, equivalent to that called in the precommit hook:

> @pnp/spfx-controls-react@3.9.0 changelog /home/dan/dev/floss/sp-dev-fx-controls-react
> node scripts/create-changelog.js && node scripts/sync-changelogs.js && gulp versionUpdater

internal/modules/cjs/loader.js:888
  throw err;
  ^

Error: Cannot find module '../CHANGELOG.json'
Require stack:
- /home/dan/dev/floss/sp-dev-fx-controls-react/scripts/create-changelog.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:885:15)
    at Function.Module._load (internal/modules/cjs/loader.js:730:27)
    at Module.require (internal/modules/cjs/loader.js:957:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at Object.<anonymous> (/home/dan/dev/floss/sp-dev-fx-controls-react/scripts/create-changelog.js:1:19)
    at Module._compile (internal/modules/cjs/loader.js:1068:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1097:10)
    at Module.load (internal/modules/cjs/loader.js:933:32)
    at Function.Module._load (internal/modules/cjs/loader.js:774:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/dan/dev/floss/sp-dev-fx-controls-react/scripts/create-changelog.js'
  ]
}

Above was tried using node 10, 14 and 16 on WSL2, all with the same results about missing ../CHANGELOG.json, although that file is present as far as I can tell.

Since the unit test results were unaffected by this PR and since this project's guidance on submitting a PR (https://pnp.github.io/sp-dev-fx-controls-react/guides/submitting-pr/) suggests that others will handle changelog and version updates, I thought it acceptable to push the branch and submit the PR.

@joelfmrodrigues joelfmrodrigues merged commit 221a2d0 into pnp:dev Jun 21, 2022
@joelfmrodrigues
Copy link
Collaborator

Hi @danwatford , many thanks for the PR, it has now been merged and will be available as part of the next release. It will also be available shortly (around 15 minutes) in the beta release, please give it a go if you have the chance.
Regarding the husky issues, you can ignore that :) Or let me know if you find the root cause as I currently have the same problem on my end 😄 I will manually update the changelog

@joelfmrodrigues joelfmrodrigues added this to the 3.9.0 milestone Jun 21, 2022
@danwatford
Copy link
Contributor Author

Hi @joelfmrodrigues , I've carried out a quick test with the beta version of the Toolbar and confirm the new Toolbar filter functionality is working.

@joelfmrodrigues
Copy link
Collaborator

@danwatford many thanks for the update

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.

None yet

2 participants