Persona: Deprecate primaryText #4811
Conversation
Add Deprecation guidelines.
|
@mtennoe , @jakob101, @Markionium The PR here is one of a few places where we render "primary text", but the prop isn't called "text". We have a few places where the "text" you provide is called different things, so we want to standardize on the "text" prop. (Obviously, |
|
Seems ok with me, i don't really see any specific reason why we would have any objections. We'll obviously have to migrate over on our side. [edit] Marius/Jakob, i created a work item on us for this :) |
|
@Markionium Please let me know if you run into any issues whatsoever with backwards compatibility. I've added as many tests on old behavior as possible to catch any issues, so if I miss anything I want to make sure I modify our process and guidelines appropriately. Thanks 😄 |
| }); | ||
|
|
||
| beforeAll(() => { | ||
| Utilities.warnDeprecations = jest.fn().mockImplementation(() => { /** no impl **/ }); |
There was a problem hiding this comment.
in my testing, commenting line 17-23 doesn't cause the test to fail..
There was a problem hiding this comment.
So the problem is you called the file .text.tsx
There was a problem hiding this comment.
Nice catch, that caught a backwards compatibility bug too. Shows the tests are working as intended 😄
ghdocs/BestPractices/Deprecation.md
Outdated
|
|
||
| ```tsx | ||
| // Prevent warn deprecations from failing test | ||
| const Utilities = require('@uifabric/utilities/lib/warn'); |
There was a problem hiding this comment.
We shouldn't be using require here. Amongst other things, Utilities will not be typed as a result.
Recommend:
import * as Utilities from '@uifabric/utilities/lib/warn';
jest.spyOn(Utilities, 'warnDeprecations').mockImplementation(() => { /** no impl **/ });The only thing I'm a little less sure about is whether there is a need to cleanup the mock after the fact, as the mock seems localized to the tests within the file/module only.
If you do a beforeEach/afterEach reset though, you can then have asserts again warnDeprecation being triggered.
There was a problem hiding this comment.
Yeah I think this should also be mockReset and not mockClear
There was a problem hiding this comment.
Does anybody know of a way to spyOn or mock exported modules from index modules like Utilities.ts? I don't like using the full path show above as most test imports are using import { setRTL } from '../../Utilities'; but I haven't found a workaround yet.
|
No objections from my side about doing this. Aligning on consistent coding names and patterns is important, and it only needs a minor change when consuming the updated version |
…i-fabric-react into parallel-tsc * 'parallel-tsc' of https://github.com/Markionium/office-ui-fabric-react: Variants: have project use OUFR instead of just styling (microsoft#4854) Add more customization hooks to ProgressIndicator (microsoft#4566) Issue#4832: Break BaseButton Types hard dependency from class ContextualMenu (microsoft#4845) Applying package updates. Revert react portals change (microsoft#4840) Update ImageOverview.md Fixes duplicate reading of suggestions on people picker (microsoft#4765) Persona: Deprecate primaryText (microsoft#4811) Experiments: Fix Fluent theme color names (microsoft#4834) Applying package updates. Add JasonGore to command bar codeowners Fix index import (microsoft#4826) Added overflowMenuProps property to CommandBar (microsoft#4818) Fluent theme: Fix imports to use relative paths (microsoft#4831) ContextualMenuItem: adding secondaryText (microsoft#4788) ComboBox: Option Performance Optimization (microsoft#4782)
* master: (95 commits) Variants: have project use OUFR instead of just styling (microsoft#4854) Add more customization hooks to ProgressIndicator (microsoft#4566) Issue#4832: Break BaseButton Types hard dependency from class ContextualMenu (microsoft#4845) Applying package updates. Revert react portals change (microsoft#4840) Update ImageOverview.md Fixes duplicate reading of suggestions on people picker (microsoft#4765) Persona: Deprecate primaryText (microsoft#4811) Experiments: Fix Fluent theme color names (microsoft#4834) Applying package updates. Add JasonGore to command bar codeowners Fix index import (microsoft#4826) Added overflowMenuProps property to CommandBar (microsoft#4818) Fluent theme: Fix imports to use relative paths (microsoft#4831) ContextualMenuItem: adding secondaryText (microsoft#4788) ComboBox: Option Performance Optimization (microsoft#4782) Marqueeselection style update (microsoft#4803) Applying package updates. FocusZone: Add the ability to stop focus from propagating outside the FocusZone (microsoft#4823) Unknown persona coin (microsoft#4809) ...
7 participants
Pull request checklist
$ npm run changeDescription of changes
Deprecate Persona's primaryText prop. Add new 'text' prop. Replace all usages in codebase. Add Deprecation guidelines.
Focus areas to test
Persona and PersonaCoin's use of primaryText and text props.