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.

Sign up for GitHub

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

Jump to bottom

Fixing virtual_packet_logging instructions for line info widget #1575

Merged
merged 1 commit into from
May 18, 2021
Merged

Fixing virtual_packet_logging instructions for line info widget #1575

merged 1 commit into from
May 18, 2021

Conversation

DhruvSondhi
Copy link
Contributor

Implements #1551 (comment)
Fixing position
cc @jaladh-singhal

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@DhruvSondhi DhruvSondhi force-pushed the virtual_packet_logging_instr branch from 6a99087 to 41abb0c Compare May 17, 2021 14:35
@codecov
Copy link

codecov bot commented May 17, 2021 •
edited
Loading

Codecov Report

Merging #1575 (782dc2e) into master (bf46269) will not change coverage.
The diff coverage is n/a.

❗ Current head 782dc2e differs from pull request most recent head 016fea8. Consider uploading reports for the commit 016fea8 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1575   +/-   ##
=======================================
  Coverage   67.19%   67.19%           
=======================================
  Files          73       73           
  Lines        6146     6146           
=======================================
  Hits         4130     4130           
  Misses       2016     2016

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf46269...016fea8. Read the comment docs.

jaladh-singhal
jaladh-singhal reviewed May 17, 2021
View reviewed changes
docs/using/visualization/generating_widgets.ipynb
@@ -574,6 +574,19 @@
"![Line Info Widget Demo](../images/line_info_widget_demo.gif)"
Copy link
Member

@jaladh-singhal jaladh-singhal May 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Looks good - just remove the heading "Enabling ...", there's no need of it since we're already using an information box. Another correction: "... must be active in order to produce virtual packets' spectrum ..." - "to produce to show" is repetitive.


Reply via ReviewNB

jaladh-singhal
jaladh-singhal requested changes May 17, 2021
View reviewed changes
Copy link
Member

@jaladh-singhal jaladh-singhal left a comment

Choose a reason for hiding this comment

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

Thanks for such quick PR - I've couple of minor comments.

@DhruvSondhi DhruvSondhi force-pushed the virtual_packet_logging_instr branch from c8a597d to 016fea8 Compare May 17, 2021 15:10
jaladh-singhal
jaladh-singhal approved these changes May 17, 2021
View reviewed changes
Copy link
Member

@jaladh-singhal jaladh-singhal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good now

@jaladh-singhal jaladh-singhal merged commit 8e5ffe1 into tardis-sn:master May 18, 2021
@DhruvSondhi DhruvSondhi deleted the virtual_packet_logging_instr branch May 22, 2021 04:42
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
@DhruvSondhi
Fixing virtual_packet_logging instructions for line info widget (tard…
ecee625 
…is-sn#1575)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaladh-singhal jaladh-singhal jaladh-singhal approved these changes

@andrewfullard andrewfullard Awaiting requested review from andrewfullard

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DhruvSondhi @jaladh-singhal @andrewfullard