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.

Sign up for GitHub

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

Jump to bottom

Essentials: Add highlight addon #17800

Merged
merged 41 commits into from
Jun 29, 2022
Merged

Essentials: Add highlight addon #17800

merged 41 commits into from
Jun 29, 2022

Conversation

winkerVSbecks
Copy link
Contributor

@winkerVSbecks winkerVSbecks commented Mar 25, 2022 •
edited
Loading

What I did

Extracted the highlight API into an own addon and added it to Essentials.

The highlight functionality started in the a11y addon but is now being used by interactions too. Extracting and documenting it will make it easier to others to use it too.

I had to update a few snapshot tests since a11yHighlight.js was removed as it's no longer required.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

highlight

highlight

@winkerVSbecks winkerVSbecks added maintenance User-facing maintenance tasks addon: essentials labels Mar 25, 2022
@nx-cloud
Copy link

nx-cloud bot commented Mar 25, 2022 •
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8396fc8. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@nx-cloud
Copy link

nx-cloud bot commented Mar 25, 2022

☁️ Nx Cloud Report

We didn't find any information for the current pull request with the commit 910cdd8.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.

Sent with 💌 from NxCloud.

shilman
shilman reviewed Mar 25, 2022
View reviewed changes
addons/highlight/README.md Outdated Show resolved Hide resolved
@winkerVSbecks
Copy link
Contributor Author

@shilman @yannbf does it make sense to have this as an addon or move it into core?

@shilman
Copy link
Member

shilman commented Mar 25, 2022

@winkerVSbecks I think it belongs as an addon even though it doesn't really do much on its own. We're trying to slim down the core, and having it as an addon keeps it separate and also it can be opt out. WDYT?

@winkerVSbecks
Copy link
Contributor Author

@shilman that makes sense. In the future, I'd like to add an overlay option which would be injected via decorators. So, an addon totally makes sense. It's in essentials so it'll be available by default in most cases

jonniebigodes
jonniebigodes requested changes Mar 25, 2022
View reviewed changes
Copy link
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@winkerVSbecks this looks amazing, fantastic job on this one! Left a couple of items to be addressed and this is good on my end.

docs/essentials/highlight.md Outdated Show resolved Hide resolved
docs/essentials/highlight.md Outdated Show resolved Hide resolved
docs/essentials/introduction.md Outdated Show resolved Hide resolved
@shilman shilman mentioned this pull request Mar 28, 2022
Closed
@winkerVSbecks
Copy link
Contributor Author

@jonniebigodes updated snippets

@jonniebigodes
Copy link
Contributor

@winkerVSbecks thanks for taking care of my feedback so promptly and including snippets for the other frameworks! On my end, this is good to go!

@winkerVSbecks
Copy link
Contributor Author

@shilman not sure why yarn bootstrap --core is erroring. It works fine locally for me.

@shilman shilman mentioned this pull request Apr 18, 2022
Merged
shilman
shilman requested changes Apr 18, 2022
View reviewed changes
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

See #17991, especially the bit about testing

@shilman
Copy link
Member

shilman commented Apr 18, 2022 •
edited
Loading

@winkerVSbecks since this is a breaking change, a few suggestions:

  • include a note in MIGRATION.md that addon-a11y users need to install addon-highlight
  • include a check in addon-a11y/etc. that addon-highlight is installed/used. addon-controls used to do this for addon-docs since docs is where the automatic argtype inference happened.
  • possibly include an automigration to add addon-highlight automatically if you're using addon-a11y/etc.

@shilman shilman closed this Apr 18, 2022
@shilman shilman reopened this Apr 18, 2022
@winkerVSbecks winkerVSbecks removed the maintenance User-facing maintenance tasks label Apr 18, 2022
@MichaelArestad MichaelArestad changed the base branch from next to future/base June 27, 2022 15:09
@MichaelArestad
Copy link
Contributor

@winkerVSbecks We updated the merge target to future/base. When you get a chance, can you reach out to @ndelangen to work through conflicts? Once conflicts are resolved, this should be good to merge.

@ndelangen
Merge branch 'future/base' into addon-highlight
3c6d2f3 
# Conflicts:
#	addons/a11y/package.json
#	addons/a11y/src/components/A11yContext.tsx
#	addons/essentials/package.json
#	examples/html-kitchen-sink/package.json
#	examples/official-storybook/main.ts
#	examples/official-storybook/package.json
#	examples/preact-kitchen-sink/package.json
#	examples/server-kitchen-sink/package.json
#	examples/svelte-kitchen-sink/package.json
#	examples/vue-kitchen-sink/package.json
#	examples/web-components-kitchen-sink/package.json
#	examples/web-components-kitchen-sink/yarn.lock
#	lib/core-server/src/__snapshots__/cra-ts-essentials_preview-dev-posix
#	lib/core-server/src/__snapshots__/cra-ts-essentials_preview-prod-posix
#	lib/core-server/src/__snapshots__/html-kitchen-sink_preview-dev-posix
#	lib/core-server/src/__snapshots__/html-kitchen-sink_preview-prod-posix
#	lib/core-server/src/__snapshots__/vue-3-cli_preview-dev-posix
#	lib/core-server/src/__snapshots__/vue-3-cli_preview-prod-posix
#	lib/core-server/src/__snapshots__/web-components-kitchen-sink_preview-dev-posix
#	lib/core-server/src/__snapshots__/web-components-kitchen-sink_preview-prod-posix
#	nx.json
#	workspace.json
#	yarn.lock
@ndelangen
Copy link
Member

I think I resolved the merge conflicts @winkerVSbecks !

@winkerVSbecks
Copy link
Contributor Author

Thanks @ndelangen. I'll take look to confirm everything is working as expected.

@winkerVSbecks
Copy link
Contributor Author

@shilman this is ready to merge

@shilman shilman added the maintenance User-facing maintenance tasks label Jun 29, 2022
@shilman
Copy link
Member

shilman commented Jun 29, 2022

@winkerVSbecks the core tests are failing. i've re-run them to see if it was a CI hiccup. if it fails a second time we should dig in to keep CI green

@shilman
Copy link
Member

shilman commented Jun 29, 2022

@winkerVSbecks also it looks like there are some CSS regressions in Chromatic. Can you please take a look?

@winkerVSbecks
Copy link
Contributor Author

@shilman they were all green then I noticed that a few of the examples didn't have addon-highlight installed. I installed and pushed and now everything breaks. I'm not sure why.

I'll check chromatic too

shilman
shilman approved these changes Jun 29, 2022
View reviewed changes
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Great job @winkerVSbecks and thanks for your patience on this!

@shilman shilman merged commit 4eefe54 into future/base Jun 29, 2022
@shilman shilman deleted the addon-highlight branch June 29, 2022 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shilman shilman shilman approved these changes

@kylegach kylegach Awaiting requested review from kylegach

@yannbf yannbf Awaiting requested review from yannbf

@jonniebigodes jonniebigodes Awaiting requested review from jonniebigodes

Assignees

@winkerVSbecks winkerVSbecks

@ndelangen ndelangen

Labels
addon: essentials maintenance User-facing maintenance tasks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@winkerVSbecks @shilman @jonniebigodes @yannbf @MichaelArestad @ndelangen