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.

Sign up for GitHub

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

Jump to bottom

Button: Getting anchor native props if href prop is specified #9456

Merged
merged 6 commits into from
Jun 18, 2019
Merged

Button: Getting anchor native props if href prop is specified #9456

merged 6 commits into from
Jun 18, 2019

Conversation

khmakoto
Copy link
Member

@khmakoto khmakoto commented Jun 13, 2019 •
edited
Loading

Pull request checklist

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

Description of changes

This PR gets anchorProperties instead of buttonProperties if the href prop has been specified for the Button component. Examples and snapshot tests have been added for this scenario as part of this PR.

I also realized that a lot of snapshot tests were not running correctly because it was missing the call to the toMatchSnapshot function, so this PR also fixes that.

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

@micahgodbolt micahgodbolt mentioned this pull request Jun 13, 2019
Closed
37 tasks
@msft-github-bot
Copy link
Contributor

msft-github-bot commented Jun 13, 2019 •
edited
Loading

Component perf results:

Scenario Target branch avg total (ms) PR avg total (ms) Target branch avg per item (ms) PR avg per item (ms) Is significant change Is regression
ScenarioDefaultButton 38.057 38.232 0.381 0.382 false false

JasonGore
JasonGore reviewed Jun 13, 2019
View reviewed changes
packages/experiments/src/components/Button/Button.view.tsx
// TODO: 'href' is anchor property... consider getNativeProps by root type
const buttonProps = { ...getNativeProps(rest, buttonProperties) };
const buttonProps = { ...getNativeProps(rest, propertiesType) };
Copy link
Member

@JasonGore JasonGore Jun 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

TS3.5 is requiring explicit types for getNativeProps calls (due to deficiencies in getNativeProps typing that I've documented there) so you may have to add one here. You can search any other use of getNativeProps for examples.

Copy link
Member

Choose a reason for hiding this comment

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

Here's an example: https://github.com/OfficeDev/office-ui-fabric-react/blob/6ff7873e60436513ce7c3e4f39982a461fa5c86f/packages/office-ui-fabric-react/src/components/Persona/PersonaCoin/PersonaCoin.base.tsx#L72

Just swap the HTMLElement typing.

@size-auditor
Copy link

size-auditor bot commented Jun 14, 2019 •
edited
Loading

Bundle test Size (minified) Diff from master
experiments-Button 88.098 kB ExceedsBaseline     131 bytes

ExceedsTolerance  Exceeds Tolerance     ExceedsBaseline  Exceeds Baseline     BelowBaseline  Below Baseline     1 kB = 1000 bytes

joschect
joschect reviewed Jun 14, 2019
View reviewed changes
packages/experiments/src/components/Button/Button.view.tsx Outdated
import { Icon } from '../../utilities/factoryComponents';

import { IButtonComponent, IButtonProps, IButtonRootElements, IButtonSlots, IButtonViewProps } from './Button.types';

export const ButtonView: IButtonComponent['view'] = props => {
const { icon, content, children, disabled, onClick, allowDisabledFocus, ariaLabel, keytipProps, buttonRef, ...rest } = props;

const rootType = _deriveRootType(props);
const htmlType = rootType === 'a' ? 'link' : 'button';
const propertiesType = rootType === 'a' ? anchorProperties : buttonProperties;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is starting to make me feel that this view is doing too much and button vs anchor should be split out into smaller components. That way you can check once if there is an href and then have each component handle it's own props correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe there should be a function like _deriveRootState or something that bundles all that work into one place.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about the second option you mentioned yesterday and I think I'm going to go ahead and combine those 3 lines into one function.

Copy link
Member Author

Choose a reason for hiding this comment

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

@joschect Updated with the function approach bundling that work 👍

@dzearing dzearing merged commit f666ee6 into microsoft:master Jun 18, 2019
@msft-github-bot
Copy link
Contributor

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@khmakoto khmakoto deleted the buttonNativeProps branch June 18, 2019 17:08
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request Jun 19, 2019
@Markionium
Merge branch 'master' into mapol/persona-dual-presence
b252b15 
* master: (46 commits)
  Enable conditional rendering of page sections depending on query present in the url to enable iframe rendering of examples on docs.microsoft OUFR portal (microsoft#9438)
  Applying package updates.
  Make more examples exportable to codepen (microsoft#9468)
  Fix create-package script (microsoft#9491)
  Card: Making elevation themable by using the effects from theme instead of directly referencing Depths (microsoft#9472)
  Revert "Tooltip: improve perf by preventing all render until after de… (microsoft#9495)
  a11y-tests: Run a11y tests on all component examples (microsoft#9479)
  Website (mobile): Add source code links, remove generic library link (microsoft#9487)
  Facepile - Adding OnRenderPersona and OnRenderPersonaCoin as optional… (microsoft#9480)
  Applying package updates.
  Accessibility improvement for DetailsList while placeholder data is being displayed (microsoft#9484)
  Button: Getting anchor native props if href prop is specified (microsoft#9456)
  Fabric website: add documentation entry points. (microsoft#9477)
  Website: Add overview and control request sections to mobile pages. (microsoft#9483)
  Fixing some styling bugs in theme designer: theme slots pivot was not centered & Main part of the page with the 3 stacks had a margin when it was unnecessary. Also cleaned up the code by creating a separate component for ThemeSlots that does the pivoting. (microsoft#9458)
  Only run KeytipManager update when relevant keytip props have changed (microsoft#9414)
  Theme colors page (microsoft#9369)
  Prevent Callout from being dismissed after mouse pressed inside but released outside (microsoft#9415)
  Add data viz separator for HorizontalBarChart (microsoft#9482)
  Applying package updates.
  ...
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JasonGore JasonGore JasonGore left review comments

@joschect joschect joschect left review comments

@KevinTCoughlin KevinTCoughlin KevinTCoughlin left review comments

@dzearing dzearing Awaiting requested review from dzearing

Assignees

@aneeshack4 aneeshack4

Labels
Component: Button Package: experiments
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@khmakoto @msft-github-bot @KevinTCoughlin @joschect @JasonGore @dzearing @aneeshack4