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: Addition of XHR/feat Breakpoints #1856

Merged
merged 11 commits into from
Nov 27, 2023

Conversation

OnesAndZer0s
Copy link
Contributor

image

@connor4312
Copy link
Member

I wonder if we could fold this into the existing Event Listener Breakpoints view to avoid add more UI clutter. Spitballing:

  • There should be a special "XHR Breakpoints" category to contain these. The category has an inline + action to add a new breakpoint
  • If no XHR breakpoints were set yet, the category has a child item to "Add new breakpoint for URL", which opens a quickpick to add the BP
  • Child items have a checkbox to enable/disable as well as an inline X action to remove them

@OnesAndZer0s OnesAndZer0s reopened this Nov 1, 2023
@OnesAndZer0s
Copy link
Contributor Author

OnesAndZer0s commented Nov 1, 2023

  • There should be a special "XHR Breakpoints" category to contain these. The category has an inline + action to add a new breakpoint

I agree, that sounds like a good idea. However, I have some qualms:

  • There is a decent change that the "XHR/Fetch Breakpoints" category would get confused with the xhr event breakpoint category.
  • I do not know if it is able to add a condition to make a Category with custom buttons within generate-contributions. Maybe something with when?

Otherwise, yeah that does sounds nice.

@connor4312
Copy link
Member

Perhaps we call them "URL XHR Breakpoints"?

Maybe something with when?

Indeed, ctrl+f for "If you want to show an action for specific tree items" on https://code.visualstudio.com/api/extension-guides/tree-view#view-actions

@connor4312
Copy link
Member

It seems like the package-lock.json was rewritten, probably with a newer version of npm. Can you undo that change? We use Node 18 and its included version of npm in our pipelines.

parent.checkboxState = state;
} else if (
enabled &&
(this.getChildren(parent) as XHRBreakpoint[]).every(
Copy link
Member

Choose a reason for hiding this comment

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

If we add parent and checked on the XHRBreakpoint then we can avoid this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

XHRBreakpoint already has checked. What would be avoiding?

Copy link
Member

@connor4312 connor4312 left a comment

Choose a reason for hiding this comment

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

looking good, a couple last comments

@OnesAndZer0s
Copy link
Contributor Author

It seems like the package-lock.json was rewritten, probably with a newer version of npm. Can you undo that change? We use Node 18 and its included version of npm in our pipelines.

Fixed.

Is there any other suggestions?

@connor4312 connor4312 enabled auto-merge (squash) November 27, 2023 22:16
@connor4312 connor4312 merged commit f1996b8 into microsoft:main Nov 27, 2023
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.

3 participants