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

Coach Mark: Fix react component positioning #638

Merged
merged 3 commits into from
Oct 19, 2023

Conversation

henryclong
Copy link
Contributor

This change: (check at least one)

  • Adds a new feature
  • Fixes a bug
  • Improves maintainability
  • Improves documentation
  • Is a release activity

Is this a breaking change? (check one)

  • Yes
  • No

Is the: (complete all)

  • Title of this pull request clear, concise, and indicative of the issue number it addresses, if any?
  • Test suite(s) passing?
  • Code coverage maximal?
  • Changeset added?
  • Component status page up to date?

What does this change address?
Coach mark react component is not correctly positioned in storybook and pharos docs

How does this change work?
computePosition method must be called after the coach mark element is added to the document in order to reference the id attribute. computePosition is called prior to rendering for the react component, resulting in a null id value (at time of calling) and a null targetElement reference. This may be related to how the web component is wrapped by the react component.

To fix this, autoUpdate() was extracted from setOffset(). autoUpdate() was adjusted to trigger a rerender and setOffset() was moved into render()

Before After
Screenshot 2023-10-18 at 4 49 33 PM Screenshot 2023-10-18 at 4 49 39 PM

@henryclong henryclong requested a review from a team as a code owner October 18, 2023 21:15
@henryclong henryclong requested review from daneah, SMQuazi and mtorres3 and removed request for a team October 18, 2023 21:15
@changeset-bot
Copy link

changeset-bot bot commented Oct 18, 2023

🦋 Changeset detected

Latest commit: 840b3ce

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@ithaka/pharos Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 18, 2023

size-limit report 📦

Path Size
packages/pharos/lib/index.js 52.75 KB (+0.02% 🔺)

@daneah daneah merged commit 29fdb15 into develop Oct 19, 2023
6 checks passed
@daneah daneah deleted the fix/coachmark-react-positioning branch October 19, 2023 14:28
@github-actions github-actions bot mentioned this pull request Oct 23, 2023
sirrah-tam pushed a commit to sirrah-tam/pharos that referenced this pull request Dec 1, 2023
* fix: ensure that react coach mark component is positioned correctly

* chore: commit changeset

---------

Co-authored-by: Dane Hillard <[email protected]>
daneah added a commit that referenced this pull request Dec 19, 2023
* develop:
  A11y revamp: Pharos buttons (non-breaking change) (#628)
  Radio, Checkbox: Fix group label (#652)
  Add elevation tokens and documentation (#643)
  fix(sidenav-link): external link opens in new tab (#645)
  Upgrade to TypeScript 5 (#644)
  feat(cli): add newly created components created using pharos-cli to initComponents files (#630)
  chore: version packages (#640)
  Coach Mark: Fix react component positioning (#638)
  Coach Mark: Documentation fixes (#639)
  chore(deps): bump @babel/traverse from 7.20.0 to 7.23.2 (#637)
  chore: version packages (#636)
  Icon: Add Panorama icon (#631)
  chore: version packages (#629)
  Loading spinner: add small and on background variant (#627)
  chore: version packages (#626)
  Sheet: allow expansion with attribute (#625)
  fix(button): remove fill on subtle disabled button on background (#618)
  chore(deps-dev): bump postcss from 8.4.25 to 8.4.31 (#624)
  chore: version packages (#623)
  Sheet: Add more close options and transition timing function (#620)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants