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.

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

Action menu text #1124

Merged
merged 11 commits into from
Feb 21, 2023
Merged

Action menu text #1124

merged 11 commits into from
Feb 21, 2023

Conversation

yerramshilpa
Copy link
Collaborator

@yerramshilpa yerramshilpa commented Feb 15, 2023

Description

This PR is for adding text and outline style for the Action Menu, this is specific to Escalation pages.

https://cm-jira.usa.gov/browse/IAEMOD-10402
Screen Shot 2023-02-15 at 1 38 35 PM

Motivation and Context

Type of Change (Select One and Apply Github Label)

  • Bug fix (non-breaking change which fixes an issue) -> Apply bugfix label
  • New feature (non-breaking change which adds functionality) -> Apply enhancement label
  • Breaking change (fix or feature that would cause existing functionality to change) -> Apply breaking label

Screenshots (if appropriate):

Which browsers have you tested?

  • Internet Explorer 11
  • Edge
  • Chrome
  • Firefox
  • Safari

Checklist:

@cwolf10
Copy link
Collaborator

cwolf10 commented Feb 15, 2023

When this becomes a normal PR, for future reference can you include at least the number of the issue this is addressing

@@ -19,6 +19,7 @@ export class SdsActionsMenuComponent {
if (!menuItem.mode) menuItem.mode = this.actionModes.SHOWN;
});
}
this.model.isIconMenu = this.model.label ? false : true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the intention is to allow devs to set this field? If so, I think this value should only be set like this if this value is not set. Otherwise there could be a scenario where the expected value gets changed by this line. If that is not the intent, this should probably be made to an internal variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed and refactored the method more generic and added story book documentation

@cwolf10
Copy link
Collaborator

cwolf10 commented Feb 16, 2023

Given we are moving to storybook, this change should be reflected in the stories there too.

Configurable should be updated to allow label to be edited, and if the intention is for isIconLabel to be passed by a dev, that should be added as a field too.

Additionally, a story with code-preview enabled would be helpful. There isn't a story currently that would make sense to update with this, so perhaps add another story to highlight these inputs.

},

table: {
category: 'ActionMenuModel/actions',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since label isnt part of the actions object, the category should be 'ActionMenuModel'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the catagory

@@ -151,4 +178,4 @@ export const Introduction: Story = (args) => ({
});
Introduction.parameters = { options: { showPanel: false } };

export const __namedExportsOrder = ['Introduction', 'Configurable', 'ModelTrigger', 'Size'];
export const __namedExportsOrder = ['Introduction', 'Configurable', 'ModelTrigger', 'Size', 'Label'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only introduction and Configurable should be out of alphabetic order. Unfortunately, there ins't a way to tell this to Storybook so we've got to manually make sure the order is right. Stick label before modelTrigger.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed to alphabetical order

@yerramshilpa yerramshilpa merged commit 913ce21 into master Feb 21, 2023
@yerramshilpa yerramshilpa deleted the action-menu-text branch October 16, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants