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

refactor(core): reactify pipelineRoles component #7104

Merged
merged 1 commit into from
Jun 11, 2019

Conversation

Jammy-Louie
Copy link

No description provided.

@Jammy-Louie
Copy link
Author

Refactored pipeline roles to React:

Screen Shot 2019-06-11 at 10 30 15 AM

Much smaller change then my last PR

Copy link
Contributor

@maggieneterval maggieneterval left a comment

Choose a reason for hiding this comment

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

LGTM, just one question about possibly over-fetching the allowed roles!

@Jammy-Louie Jammy-Louie merged commit a3e678e into spinnaker:master Jun 11, 2019
@Jammy-Louie Jammy-Louie deleted the reactify-allowedRoles branch June 11, 2019 16:06
export const PipelineRoles = (props: IPipelineRolesConfigProps) => {
const [allowedRoles, setAllowedRoles] = useState([]);

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need a useEffect at all... since getAuthenticatedUser() is synchronous you can either:

  1. simply grab the value each time
  2. cache it using useMemo
    const allowedRoles = useMemo(() => AuthenticationService.getAuthenticatedUser().roles, []);

anotherchrisberry added a commit that referenced this pull request Jun 11, 2019
0686fd8 fix(core): fix auto-navigation on route=true searches (#7092)
a3e678e refactor(core): reactify pipelineRoles component (#7104)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants