Skip to content

Conversation

@KevinTCoughlin
Copy link
Member

@KevinTCoughlin KevinTCoughlin commented Sep 10, 2018

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

There's currently a runtime error thrown in <LayerBase> due to a missing import setPortalAttribute

image

It looks like the fix is to mark it @public so it can be properly imported?

Focus areas to test

  1. Run npm start in this branch
  2. Navigate to something using <LayerBase/> such as <DatePicker/>.
  3. The page shouldn't crash as it does when you try to open the DatePicker on the examples site.
Microsoft Reviewers: Open in CodeFlow

@KevinTCoughlin
Copy link
Member Author

KevinTCoughlin commented Sep 10, 2018

Prettier had opinions about dom.ts

/**
* Identify element as a portal by setting an attribute.
* @param element Element to mark as a portal.
* @public
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the "fix"

Copy link
Member

@JasonGore JasonGore Sep 10, 2018

Choose a reason for hiding this comment

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

I'm confused how a jsdoc tag removes a runtime error? Are you sure this error wasn't due to a local build error (i.e. utilities package was out of date)? I've never seen this error while developing and testing portals and don't see it locally now.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're probably right that it was rush and my local setup. Closing this with the real focus fix in #6308

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
@KevinTCoughlin KevinTCoughlin deleted the keco/mark-setPortalAttribute-public branch January 2, 2020 19:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants