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

feat(core): add one click details information popover for failed pipeline stage #8325

Conversation

RodEsp
Copy link
Contributor

@RodEsp RodEsp commented Jun 3, 2020

Copy of #8322, but rebased on and PR'ed against master.

feat(execution/ExecutionMarkerInformationPopover) Adds 1-click access to the actual error at the last level pipeline

  • New feature can be enabled/disabled via executionMarkerInformationPopover in the settings.js spinnakerSettings:feature section
  • ExecutionMarker.tsx now loads the new component and for a stage with status 'terminal', type 'pipeline' and will add an font-awesome information circle icon to the stage
  • When information icon is clicked modal will load and traverse through the execution => terminal stage(pipeline) until the last instance is found then report that actual error stage and execution allowing for quick access and viewing
  • ExecutionMarkerInformationPopover component adds/removes listeners for the window.resize and '.all-execution-groups' scroll events to allow for updating it's position and 'nubbin' position, scrolling the clicked stage as it hit the stop container will close
  • ExecutionMarkerInformationPopoverState caches and validates api calls returning cached versions for quicker access since there can be many calls made to the same execution/pipelineConfigs etc
  • Lists the execution history in reverse order as clickable rows
  • Has secondary area for showing history again in reverse order using the spinnaker PipelineGraph component

image
image

Shaunery Seale and others added 2 commits June 3, 2020 13:58
New feature can be enabled/disabled via executionMarkerInformationPopover in the settings.js spinnakerSettings:feature section
ExecutionMarker.tsx now loads the new component and for a stage with status 'terminal', type 'pipeline' and will add an font-awesome information circle icon to the stage
When information icon is clicked popover will load and traverse through the execution => terminal stage(pipeline) until the last instance is found then report that actual error stage and execution allowing for quick access and viewing
popover is not using jquery or bootstrap but does use the react-bootstrap css classes
ExecutionMarkerInformationPopover component adds/removes listeners for the window.resize and '.all-execution-groups' scroll events to allow for updating it's position and 'nubbin' position, scrolling the clicked stage as it hit the stop container will close
ExecutionMarkerInformationPopoverState caches and validates api calls returning cached versions for quicker access since there can be many calls made to the same execution/pipelineConfigs etc
Lists the execution history in reverse order as clickable rows
Has secondary area for showing history again in reverse order using the spinnaker PipelineGraph component
@spinnakerbot
Copy link
Contributor

The following commits need their title changed:

  • e86839c: Adds 1-click access to the actual error at the last level pipeline

  • a7320d8: Add executionId to the IExecutionViewState interface.

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

@maggieneterval maggieneterval self-requested a review June 3, 2020 18:51
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.

🎉 I built your branch locally and played around with this a bit, and it looks 💯! Thanks for the excellent work!

Besides several questions and codestyle nits I've left inline, my biggest concern is around the fragile logic in ExecutionMarkerInformationPopover for adjusting the modal based on resize/scroll events. Given that other modals in Spinnaker are not responsive in this way, I'm hesitant to add a lot of complexity to this one particular component that seems likely to unexpectedly break if someone were to, for example, refactor its CSS. Do we feel this would be usable without such resize/scroll responsiveness?

Thanks again for the great contribution!

@sseale-zz
Copy link
Contributor

@maggieneterval I fixed up issues in commit f508b64 and added some inline comments to help with the "why's" of some of the code let me now what you think.

@RodEsp

@maggieneterval
Copy link
Contributor

Thanks so much, @sseale and @RodEsp!

Apologies for not thinking of this right away, but how would you feel about removing the custom resize/scroll handling and instead placing this component inside Deck's existing Modal component? I think it would be preferable in terms of visual consistency as well.

The other pre-existing component Deck has for opening content in the foreground above the main view is the HoverablePopover. This also handles placement in relation to the anchor element, but automatically opens/closes on hover start/end, so likely would be less than ideal for a large component like this from which you expect users to need to click into other elements.

Let me know what you think!

@maggieneterval
Copy link
Contributor

And one final minor issue I just discovered while running the latest version of your branch is the following warning about non-unique keys:

Warning: Each child in a list should have a unique "key" prop.

Check the render method of `ExecutionMarkerInformationPopover`.

@RodEsp
Copy link
Contributor Author

RodEsp commented Jun 8, 2020

Heya @maggieneterval, will definitely take care of that unique key warning.

I was playing around last week with re-implementing this not with the HoverablePopover because "hover" won't work for our usecase but with the react-boostrap OverlayTrigger and Popover components (which is what HoverablePopover uses) and I was having a lot of trouble trying to get things to line up properly with the anchor element.

Thanks for the Modal suggestion! I'm going to try that and keep playing around with this a little more. I'll get back to you in a day or two and let you know if it works or what my findings were if it doesn't.

@sseale-zz
Copy link
Contributor

@maggieneterval @RodEsp we can try to play around with the modal and just have it open and not worry about all the event handlers. Each stage is already a Tooltip component so not sure how well wrapping a tooltip with a tooltip/popover works. Sorry i thought i added the new key in to fix that but realized i added it to our custom version in 1.19. We will get that fixed as well.

@sseale-zz
Copy link
Contributor

@maggieneterval this is now using the Modal
Screen Shot 2020-06-09 at 3 05 51 PM

start.sh Outdated Show resolved Hide resolved
Shaunery Seale added 3 commits June 15, 2020 13:37
renamed files to modal from popover
renamed state file to service
added service spec skipped tests for now, not sure what i'm doing
fixed a few issue in pr
@sseale-zz
Copy link
Contributor

@maggieneterval i have made some changes but have not figure out anything with the ui router component wrapped around an tr as i mentioned above. i added in a service spec but skipping the two tests for now.

Shaunery Seale added 5 commits June 16, 2020 11:56
runs fine locally doesn't in github => testing
…ailed_pipeline_stage' of github.com:sseale/deck into feature/add_one_click_details_information_popover_for_failed_pipeline_stage
each td now have a UISref component in it
@sseale-zz
Copy link
Contributor

sseale-zz commented Jun 16, 2020

@maggieneterval i believe i have all the issues addressed now

@maggieneterval maggieneterval self-requested a review June 17, 2020 21:23
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.

Pending resolution of a few remaining comments, would love to merge this by end of day Friday so it makes it in time for the 1.21 release.

If anyone else who saw this demo'd at the UI SIG (@caseyhebebrand @erikmunson @gcomstock) wants to take a pass as well before we merge, please feel free!

@maggieneterval maggieneterval added the ready to merge Reviewed and ready for merge label Jun 19, 2020
@maggieneterval maggieneterval changed the title Feature/add one click details information popover for failed pipeline stage feat(core): add one click details information popover for failed pipeline stage Jun 19, 2020
@mergify mergify bot merged commit a2af93f into spinnaker:master Jun 19, 2020
@RodEsp
Copy link
Contributor Author

RodEsp commented Jun 19, 2020

Thank you so much with all your help on this Maggie!

@gcomstock
Copy link
Contributor

Thanks for the demo and all your work on this, @RodEsp ! I don't have any feedback that should bar this from being merged. I would be happy to pass along how users receive it over at netflix though, and once I see a lot of real data flow through it, I might pop back around with the design feedback if needed. Thanks again!

@RodEsp
Copy link
Contributor Author

RodEsp commented Jun 22, 2020

Sounds great, thanks @gcomstock, although @sseale did the vast majority of the work here :)

Would love to get feedback on what Netflix thinks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Reviewed and ready for merge target-release/1.21
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants