-
Notifications
You must be signed in to change notification settings - Fork 227
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
Conversation
There was a problem hiding this 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.
- Just a friendly advice: Read the hints about writing good commit messages linked from this current draft written by @formatc1702 : https://github.com/formatc1702/WireViz/blob/feature/syntax-description/docs/CONTRIBUTING.md
- A couple of my code comments below are not attached to the correct code line because github doesn't allow attaching comments to code lines too far away from code changes in this PR.
b386215
to
d5fb402
Compare
Many thanks for your valuable feedback @kvid ! |
There was a problem hiding this 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.
04c83b4
to
df625bf
Compare
There was a problem hiding this 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 withpython 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 anmpn
link and either amanufacturer
main page or some generic link in (part of) thedescription
. - 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.
9967202
to
a86a555
Compare
Many thanks for the review again @kvid !
Maybe we should add this info to the contribution guidelines? |
There was a problem hiding this 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).
As per @kvid's suggestion, this should be the next PR to be merged into Therefore, please rebase onto 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.
38982a5
to
a3345de
Compare
Hi @formatc1702 I have just rebased and squashed my changes to my branch to the dev. |
Thank you for your contribution! :) |
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.
Results:
BTW. it might be useful to add some links to some examples just to make some attention on this feature.