-
Notifications
You must be signed in to change notification settings - Fork 15
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
Mfrank/rename variables #17
Conversation
fallback: PropTypes.node, | ||
scrolling: PropTypes.oneOf(['auto', 'yes', 'no']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This attribute has been deprecated for iframes. I think it may have been causing some of the scrollbars we were seeing before.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#deprecated_attributes
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #17 +/- ##
=======================================
Coverage 75.19% 75.19%
=======================================
Files 10 10
Lines 129 129
Branches 19 19
=======================================
Hits 97 97
Misses 30 30
Partials 2 2 ☔ View full report in Codecov by Sentry. |
9d61426
to
0523922
Compare
src/example/example.env.config.js
Outdated
- runtime config | ||
*/ | ||
|
||
/** TODO: Examples still need to be set up as part of APER-3042 https://2u-internal.atlassian.net/browse/APER-3042 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this TODO and commented out code in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point on this commented out code in here. Could a lot of this exist in the README since there's already a section there about how the config should be set up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good call outs. I will move it to the README. I blame my mucus infused brain for attempting to push a full file of comments 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me but I'm not as plugged into the FPF as others on the team. Feel free to wait for a more experienced approval from someone. :)
docs: update readme docs: update readme
e25b772
to
5eec110
Compare
Updates
plugins
topluginSlots
in theenv.config.js
configuration. The only place where this value is explicitly referenced is in theusePluginSlot
hook.Some inline documentation has been added to help explain some of the
props
of the framework components. Additionally, thescrolling
attribute has been removed as it is a deprecated attribute for iframes.APER-3046