Skip to content

Conversation

sergeibbb
Copy link
Member

@sergeibbb sergeibbb commented Nov 18, 2024

Closes #3774

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
  • My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

Copy link
Contributor

@axosoft-ramint axosoft-ramint left a comment

Choose a reason for hiding this comment

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

Two tiny, very quick suggestions. Works in testing, so I think once these are addressed we're good to merge it.

Comment on lines 175 to 177
if (!isStateWithType(state)) {
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably throw here and possibly include a debugger line if we don't have a type by this point, because something must have gone very wrong. Maybe we can use a function similar to assertsLaunchpadStepState?

Copy link
Member Author

@sergeibbb sergeibbb Nov 20, 2024

Choose a reason for hiding this comment

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

In fact it just fighting the type-system. Because even if I assign state.type right before the function call it does not recognize the type:

image

But at the same time this works well:

const type: string = state.type
image

Copy link
Member Author

Choose a reason for hiding this comment

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

return md5(item.item.issue.id);
}

function buildItemActionTelemetryData(item: StartWorkItem) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: this is used outside of "action" contexts, and the action property is really only being set in sendItemActionTelemetry, so maybe we just call this buildItemTelemetryData?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@sergeibbb sergeibbb force-pushed the 3774-start-work-show-different-globe-tooltips branch from 471eb0e to 5724a60 Compare November 21, 2024 13:26
@sergeibbb sergeibbb merged commit e72be11 into main Nov 21, 2024
3 checks passed
@sergeibbb sergeibbb deleted the 3774-start-work-show-different-globe-tooltips branch November 21, 2024 13:29
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.

Start Work: show different "globe" tooltips on GitHub and Jira issues

3 participants