Skip to content

Conversation

@maximus12793
Copy link
Contributor

Pull request checklist

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

Description of changes

Add secondaryText to ContextualMenuItem to render on the right of item.name (text).

Focus areas to test

(optional)

@maximus12793 maximus12793 requested a review from joschect as a code owner May 5, 2018 07:53
@msftclas
Copy link

msftclas commented May 5, 2018

CLA assistant check
All CLA requirements met.

@maximus12793
Copy link
Contributor Author

@martellaj per our email

@dzearing dzearing changed the title Maroquem/customdata ContextualMenuItem: adding secondaryText to render to the right of the name. May 6, 2018
@dzearing dzearing changed the title ContextualMenuItem: adding secondaryText to render to the right of the name. ContextualMenuItem: adding secondaryText May 6, 2018
"sourceMaps": true
},
{
"type": "node",
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to check this in? I'm guessing so but I just wanted to make sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, I think this was just from testing with jest. Will revert this

@micahgodbolt
Copy link
Member

we need to get a visual test capturing what this new text looks like

@betrue-final-final
Copy link
Member

Yes, let's do that before we merge this. @maximus12793, is there a designer you worked with?

@maximus12793
Copy link
Contributor Author

@betrue-final-final @micahgodbolt yes Drew.

@betrue-final-final
Copy link
Member

Andrew Peacock?

@maximus12793
Copy link
Contributor Author

yes

/**
* Style for the description text if applicable (for compound buttons.)
*/
secondaryText?: IStyle;
Copy link
Member

Choose a reason for hiding this comment

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

secondary text is here, but isn't being used in button.

If the idea is to replace description with secondaryText, lets start supporting both (in BaseButton.tsx and style files) so that we can deprecate description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, sorry no I forgot to remove this. Am adding vr-tests now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought I am not sure. Without this addition I am unable to add this property to ContextualMenu.styles.ts (since this derives from IMenuStyles which falls under ButtonTypes.ts (namely, IMenuItemStyles extends IButtonStyles). Is this fine? Or how do you suggest adding it properly to just effect ContextualMenuItems?

@betrue-final-final
Copy link
Member

@peacokle, can you add some pictures of what this would look like?

@maximus12793
Copy link
Contributor Author

image

@betrue-final-final This is taken from the ContextualMenu ran via. npm start. Please feel free to run and examine on your end.

},
secondaryText: {
color: theme.palette.neutralTertiary,
paddingLeft: '20px',
Copy link
Member

Choose a reason for hiding this comment

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

@dzearing / @joschect How is RTL handled in Fabric? Is this handled in the build pipeline?

Copy link
Member

Choose a reason for hiding this comment

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

It is handled automatically.

You can test it through npm start by going to the top right gear, and selecting the rtl option :)

Copy link
Member

Choose a reason for hiding this comment

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

Cool! Thanks, @dzearing!

{
key: 'newItem',
name: 'New Button',
customData: <span className='applez'>Thu. 10:00 PM</span>
Copy link
Member

Choose a reason for hiding this comment

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

Is this class name actually used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing.

import { IContextualMenuItemProps } from 'office-ui-fabric-react/lib/ContextualMenu';
import './ContextualMenuExample.scss';

export class ContextualMenuWithCustomDataAfterNameExample extends React.Component {
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to check in this example? This looks like an exploration with the first thing you were trying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops yes am removing this

@maximus12793
Copy link
Contributor Author

Are there any additional changes we would like to see, or is this ready to go?

@martellaj
Copy link
Member

I'm going ahead and merging this since we have design sign-off from @betrue-final-final and code owner (@joschect) sign-off as well.

@martellaj martellaj merged commit 5eaf2e1 into microsoft:master May 9, 2018
@maximus12793 maximus12793 deleted the maroquem/customdata branch May 9, 2018 22:22
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request May 13, 2018
…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)
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request May 13, 2018
* 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)
  ...
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
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.

7 participants