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

54 connector widget special chars #83

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

bryannho
Copy link
Contributor

@bryannho bryannho commented Sep 25, 2023

Describe your changes

  • Fixed bug that broke connect and delete button in ConnectorWidget when special characters were included in the alias:
    • In src/widgets/connector.ts, each connection's alias was being used in the ID for its connect and delete buttons. When special characters were included in the alias, this caused an exception when document.querySelector was called on the ID.
    • To fix, I added src/util/helper-functions.ts to hold a string formatting function for special characters. This will replace special characters with their ASCII values (e.g. ':' -> '58'). I applied this function to the ID's for each connection's connect and delete buttons. Now when querySelector is called, the ID is always a valid selector.

Issue number

Closes #54

Checklist before requesting a review

@bryannho
Copy link
Contributor Author

CI seems to be failing for unknown reason, possibly related to issue #78

@bryannho bryannho force-pushed the 54-connector-widget-special-chars branch from 66e96d7 to 7c73653 Compare September 27, 2023 22:45
@bryannho bryannho marked this pull request as ready for review September 27, 2023 23:34
@edublancas
Copy link
Contributor

@bryannho is this ready? don't forget to explicitly ask for a review. are you able to request reviews?

image

if you can't, you can tag me and say "ready for review" so I get notified

@bryannho
Copy link
Contributor Author

@bryannho is this ready? don't forget to explicitly ask for a review. are you able to request reviews?

image

if you can't, you can tag me and say "ready for review" so I get notified

Apologies - this is ready to review. I marked the branch as ready but I'm not able to request reviews in the sidebar. Will tag you next time

Copy link
Contributor

@edublancas edublancas left a comment

Choose a reason for hiding this comment

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

a few things are missing:

  • changelog entry
  • explain your solution in the PR description
  • unit tests

@@ -0,0 +1,15 @@
export const specialToASCII = (str: string): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a docstring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bryannho
Copy link
Contributor Author

bryannho commented Oct 2, 2023

a few things are missing:

  • changelog entry
  • explain your solution in the PR description
  • unit tests

Added changelog, PR description, and unit tests

@edublancas
Copy link
Contributor

@bryannho what's the status here? are you gonna tackle the CI issue first or will you work on passing the CI here first?

@bryannho
Copy link
Contributor Author

bryannho commented Oct 2, 2023

@bryannho what's the status here? are you gonna tackle the CI issue first or will you work on passing the CI here first?

I'm working on passing the CI here first. Once I get that done I'll mark it for your review and start working on the CI issue. Apologies I've been working through some hardware issues with my machine but I should be have this ready for review by tomorrow morning!

@edublancas
Copy link
Contributor

sounds good!

@bryannho
Copy link
Contributor Author

bryannho commented Oct 3, 2023

@edublancas I was able to get my own individual tests to work, but the PR keeps failing checks as a result of the CI issue with multiple tries. Might need to pivot to fixing the CI issue first and then come back around to this PR.

@bryannho bryannho force-pushed the 54-connector-widget-special-chars branch from 4795a29 to 125862c Compare October 4, 2023 01:39
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.

ConnectorWidget connect or delete button doesn't work with alias that has special characters
2 participants