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

Remove HTML links from the input attributes #164

Merged
merged 1 commit into from
Oct 22, 2020

Conversation

martonmiklos
Copy link
Contributor

This change will remove HTML links from the input attributes when generating graphs with GraphViz (because it does not support this HTML tag).

This way these links could be used generated the HTML BoM table (basically without changing the input) where they could be useful.

# Example 7: Crossover Cable
connectors:
  X1:
    type: '<a href="https://www.digikey.com/product-detail/en/stewart-connector/SS-37000-002/380-1098-ND/1033365">Stewart Connector SS-37000-002</a>'
    subtype: male
    pinlabels: [DA+,DA-,DB+,DC+,DC-,DB-,DD+,DD-] # pincount is implicit in pinout
  X2:
    type: '<a href="https://www.digikey.com/product-detail/en/stewart-connector/SS-37000-002/380-1098-ND/1033365">Stewart Connector SS-37000-002</a>'
    subtype: male
    pinlabels: [DB+,DB-,DA+,DD+,DD-,DA-,DC+,DC-]

Results:
kép

BTW. it might be useful to add some links to some examples just to make some attention on this feature.

Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

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

@martonmiklos Thank you for contributing. I'm just another contributor, like you. @formatc1702 is the owner that approves and merges PRs into dev when he thinks they are ready. He is currently on vacation, and I decided to give you a few advices that I believe will help you getting this PR accepted, but remember that @formatc1702 might not share all my opinions.

src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/wv_helper.py Outdated Show resolved Hide resolved
src/wireviz/wv_helper.py Outdated Show resolved Hide resolved
src/wireviz/wv_helper.py Show resolved Hide resolved
@martonmiklos martonmiklos force-pushed the add_html_link_support branch 2 times, most recently from b386215 to d5fb402 Compare September 10, 2020 08:44
@martonmiklos
Copy link
Contributor Author

Many thanks for your valuable feedback @kvid !
I agree and fixed with all of your comments.

Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

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

Many thanks for your valuable feedback @kvid !
I agree and fixed with all of your comments.

Thank you for accepting my suggestions. See my single code comment below for a minor new issue.

BTW. it might be useful to add some links to some examples just to make some attention on this feature.

I suggest you add a link (or two) to one of the existing examples using templates to repeat using the same connector/cable, e.g. ex05.yml if you want to add links to the type attribute, or tutorial08.yml if you want to add links to the mpn attribute.

src/wireviz/Harness.py Outdated Show resolved Hide resolved
@martonmiklos martonmiklos force-pushed the add_html_link_support branch 2 times, most recently from 04c83b4 to df625bf Compare September 11, 2020 07:19
Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

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

Thank you again that you accepted my suggestions. I'm really sorry for nagging on you with even more suggestions:

  • I believe @formatc1702 prefer that you avoid committing changes to generated files in PRs to reduce merging conflicts - see Add support for Multicolored Wires #17 (comment). If you use a Linux-shell or something similar, then you can execute this command to restore the generated files from the dev branch:
    git checkout dev {examples/ex05,tutorial/tutorial08}.{gv,bom.tsv,png,svg,html}
  • Personally, I always execute python build_examples.py restore before adding and committing to avoid including changes to generated files after testing with python build_examples.py.
  • I suggest you also demonstrate hyperlinks from something in additional_bom_items in tutorial 8 to show that is supported too. If you find a real label product, adapt the entry for that, or find a totally different entry with hyperlinks that you can insert in addition to the labels entry. If it makes sense, consider also demonstrating two links for the same entry, e.g. both an mpn link and either a manufacturer main page or some generic link in (part of) the description.
  • I do like that you improved your commit message, and I don't like to criticize that again, but have you noticed that github is truncating your subject line with an ellipsis? That is a hint telling you that the subject line is longer than recommended. It might be hard to find a short subject line sometimes, but e.g. "Remove input text hyperlinks except in the HTML BOM" is a shorter alternative (within the 50 characters soft limit). I believe BOM is more common than BoM, but you decide what to write.

@martonmiklos martonmiklos force-pushed the add_html_link_support branch 2 times, most recently from 9967202 to a86a555 Compare September 12, 2020 19:01
@martonmiklos
Copy link
Contributor Author

Many thanks for the review again @kvid !

I believe @formatc1702 prefer that you avoid committing changes to generated files in PRs to reduce merging conflicts - see #17 (comment).

Maybe we should add this info to the contribution guidelines?

Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

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

Many thanks for the review again @kvid !

Thank's again for accepting my latest suggestions. In my opinion, this PR is now ready for merge into dev, but @formatc1702 might disagree.

I believe @formatc1702 prefer that you avoid committing changes to generated files in PRs to reduce merging conflicts - see #17 (comment).

Maybe we should add this info to the contribution guidelines?

I fully agree with you, and included this in my #111 (comment).

@formatc1702
Copy link
Collaborator

As per @kvid's suggestion, this should be the next PR to be merged into dev.

Therefore, please rebase onto dev to get rid of any merge conflicts, and have a look if anything needs changing.

I will squash this PR into a single commit since the actual code change is very minor, so no need to interactively rebase/restructure the commit history, IMHO.

Thanks!

GraphViz does not support the a HTML tag when generating the tables for the
cables/connectors, so this change will remove these tags for the graph generation.
However for the HTML BOM output table these links will be generated.
@martonmiklos martonmiklos force-pushed the add_html_link_support branch from 38982a5 to a3345de Compare October 22, 2020 21:02
@martonmiklos
Copy link
Contributor Author

Hi @formatc1702

I have just rebased and squashed my changes to my branch to the dev.

@formatc1702 formatc1702 merged commit e2e8bbf into wireviz:dev Oct 22, 2020
@formatc1702
Copy link
Collaborator

Thank you for your contribution! :)

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