Merged
Conversation
layershifter
requested review from
dvdzkwsk,
dzearing,
jurokapsiar,
levithomason,
miroslavstastny,
mnajdova and
pompomon
as code owners
layershifter
changed the title
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f83a850:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 6a33fd5c4919b378ce306c1877aa52d384a91019 (build) |
Contributor
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
layershifter
force-pushed
the
chore/no-with-safe-type-as
branch
from
e3383f0 to
8ee7551
Compare
layershifter
commented
Jun 30, 2020
| <List styles={listStyle}> | ||
| {/* Heads up! Still render empty lists to reserve the whitespace */} | ||
| <List.Item> | ||
| <List.Item index={0}> |
Member
Author
There was a problem hiding this comment.
index is a required prop on ListItem for Children API, microsoft/fluent-ui-react#2207
Previous typings were not correct because we exported ListItemProps only:
layershifter
commented
Jun 30, 2020
|
|
||
| <p> | ||
| <Input | ||
| ref="input" |
Member
Author
There was a problem hiding this comment.
Strings can be used only for deprecated refs API: https://reactjs.org/docs/refs-and-the-dom.html#legacy-api-string-refs. But there is no such usage 🧛
layershifter
commented
Jun 30, 2020
| import CustomAvatar from './CustomAvatar'; | ||
| import { AcceptIcon } from '@fluentui/react-icons-northstar'; | ||
|
|
||
| const statusProps: Extendable<WithAsProp<StatusProps>> = { |
Member
Author
There was a problem hiding this comment.
There is no sense in it as as is not used inside
layershifter
commented
Jun 30, 2020
| hasContent: !!content, | ||
| canBeCollapsed, | ||
| as, | ||
| as: String(as), |
Member
Author
There was a problem hiding this comment.
String is expected in the behavior
layershifter
commented
Jun 30, 2020
| /** | ||
| * A Portal allows to render children outside of their parent. | ||
| */ | ||
| const Portal: React.FC<WithAsProp<PortalProps>> & FluentComponentStaticProps<PortalProps> = props => { |
Member
Author
There was a problem hiding this comment.
Portal does not have ElementType...
layershifter
commented
Jun 30, 2020
| onOutsideClick?: (e: React.MouseEvent) => void; | ||
| } | ||
|
|
||
| export interface PortalState { |
layershifter
force-pushed
the
chore/no-with-safe-type-as
branch
from
8ee7551 to
70c52af
Compare
layershifter
added
the
Fluent UI react-northstar (v0)
layershifter
changed the title
…ithub.com/microsoft/fluentui into chore/no-with-safe-type-as � Conflicts: � packages/fluentui/react-northstar/src/components/Grid/Grid.tsx
| * [Treeview - JAWS doesn't narrate position for each tree item](https://github.com/FreedomScientific/VFO-standards-support/issues/338) | ||
| * [Aria compliant trees are read as empty tables](https://bugs.chromium.org/p/chromium/issues/detail?id=1048770) | ||
| */ | ||
| const Tree: ComponentWithAs<'div', TreeProps> & |
Member
There was a problem hiding this comment.
The change from ul to div is intentional bugfix, correct?
Member
Author
There was a problem hiding this comment.
Yep, as Tree and its subcomponents render divs:
I also double checked this with @jurokapsiar and we will avoid ul/li as they are breaking markup once you add custom scrollbar or virtualization
packages/fluentui/react-northstar/src/components/Dropdown/Dropdown.tsx
Outdated
Show resolved
Hide resolved
miroslavstastny
approved these changes
Jul 2, 2020
…down.tsx Co-authored-by: Miroslav Stastny <mistastn@microsoft.com>
Contributor
|
🎉 Handy links: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request checklist
$ yarn changeDescription of changes
This PR replaces
withSafeTypeForAs()calls with usage to:Focus areas to test
(optional)