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

Replacing the contents of the news section with TARDIS twitter feed #1555

Merged
merged 4 commits into from
Jun 4, 2021

Conversation

atharva-2001
Copy link
Member

@atharva-2001 atharva-2001 commented Apr 26, 2021

Fixes #1545

Description

  • The news section in the documentation is pretty outdated, this PR replaces its contents with a Twitter Timeline Widget.
  • Uses JS to customize the layout of the widget as mentioned here.
  • Changes the font of the News title

Motivation and context

Fixes #1545
See also: #1743

How has this been tested?

  • Testing pipeline.
  • Other.

This is working correctly in my documentation.

Examples

Before:
image

After:
image

Displays a message it not loaded:
image

Type of change

  • Bug fix.
  • New feature.
  • Breaking change.
  • None of the above.

Checklist

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
    • (optional) I have built the documentation on my fork following the instructions.
  • I have assigned and requested two reviewers for this pull request.

@atharva-2001 atharva-2001 changed the title adding tardis tweets to news feed Replacing the contents of the news section with TARDIS twitter feed Apr 26, 2021
@andrewfullard
Copy link
Contributor

I see this on Firefox
image

@codecov
Copy link

codecov bot commented Apr 26, 2021

Codecov Report

Merging #1555 (c08b553) into master (7eca324) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head c08b553 differs from pull request most recent head 8455a39. Consider uploading reports for the commit 8455a39 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1555      +/-   ##
==========================================
- Coverage   67.20%   67.20%   -0.01%     
==========================================
  Files          73       73              
  Lines        6147     6150       +3     
==========================================
+ Hits         4131     4133       +2     
- Misses       2016     2017       +1     
Impacted Files Coverage Δ
...dis/montecarlo/montecarlo_numba/numba_interface.py 49.62% <0.00%> (-0.38%) ⬇️
tardis/tardis/montecarlo/montecarlo_numba/base.py 29.62% <0.00%> (ø)
...rdis/montecarlo/montecarlo_numba/tests/conftest.py 91.11% <0.00%> (ø)
...dis/montecarlo/montecarlo_numba/tests/test_base.py 100.00% <0.00%> (ø)
tardis/tardis/montecarlo/base.py 88.64% <0.00%> (+0.06%) ⬆️
tardis/tardis/montecarlo/packet_source.py 97.29% <0.00%> (+0.07%) ⬆️

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 7eca324...8455a39. Read the comment docs.

@atharva-2001
Copy link
Member Author

I too see it at first, but it is replaced quickly by the widget, even in firefox. Is it persistent in your case?
Peek 2021-04-26 21-09

@andrewfullard
Copy link
Contributor

andrewfullard commented Apr 26, 2021

The link persists for me. This happens in incognito mode, if I refresh the page, if I clear the page cache, and if I disable plugins.

Works for me in MS Edge

The bug happens in Firefox 88.0 Windows 10

@atharva-2001
Copy link
Member Author

Ah, I'm sorry, I got it now. I was able to reproduce it in incognito mode in Firefox. It seems Firefox blocks some of the resources that are required to produce the widget.
I was able to see the widget in incognito mode after disabling the tracking protection by clicking the shield icon on the top left corner.

Screenshot from 2021-04-26 21-42-57
Does doing this fix it for you?
Reference:
https://stackoverflow.com/questions/39317653/twitter-widgets-not-working-on-firefox-private-browser
https://twittercommunity.com/t/help-embedded-twitter-widget-not-working-in-latest-firefox/57828/4

@andrewfullard
Copy link
Contributor

Yes, works for me. Unfortunate, but I'm not sure there's anything we can do about it?

@atharva-2001
Copy link
Member Author

How about displaying a message saying something like "you need to disable tracking protection to see the widget"? I am not sure how that can be done at the moment, though.

This might be useful to us:
https://stackoverflow.com/a/35070265/11974464
https://stackoverflow.com/questions/34294989/impossible-to-use-a-twitter-timeline-in-firefox

@atharva-2001
Copy link
Member Author

atharva-2001 commented Apr 29, 2021

@andrewfullard I wrote some code to check if the widget is not loaded. If it detects this, it shows a message:
image

Please check it here.
It is working in firefox for me. Could you please tell me if it works for you too?

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.

This is an excellent addition. Thank you for doing the extra effort to customize the styles and for handling the JS-disabled issue.

Once you address my comments, it's ready to be merged.

docs/index.rst Outdated Show resolved Hide resolved
docs/news.rst Outdated
jQuery('.twitter-block').find('.twitter-timeline').contents().find('.timeline-Header-title.u-inlineBlock').children().css('font-size', '18px');
jQuery('.twitter-block').find('.twitter-timeline').contents().find('.timeline-Tweet-text').css({
'font-size': '16px',
'font': 'Lato,proxima-nova,Helvetica Neue,Arial,sans-serif;',
Copy link
Member

Choose a reason for hiding this comment

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

It will be 'font-family': 'Lato,proxima-nova,Helvetica Neue,Arial,sans-serif', instead - that's why fonts didn't appear to be changed.

docs/news.rst Outdated Show resolved Hide resolved
docs/news.rst Outdated Show resolved Hide resolved
@atharva-2001
Copy link
Member Author

Thank you for reviewing it! I will get it done as soon as possible.

@atharva-2001
Copy link
Member Author

@jaladh-singhal
I am terribly sorry for such a late response. Could you please check it now?

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 changing jquery to vanilla JS - there's one small thing I've commented.

Also, I'm not able to see doc preview - link shows 404, can you build the docs by pushing your commit prefixed with [build docs]?

docs/news.rst Outdated
var index;
for (let index = 0; index < tweetTextList.length; index++) {
tweetTextList[index].style.fontSize = "16px";
tweetTextList[index].style.font = "Lato,proxima-nova,Helvetica Neue,Arial,sans-serif;";
Copy link
Member

Choose a reason for hiding this comment

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

Most probably it won't work - have you checked the font being used by web inspector on built webpage?

You should instead do this (read here]:

Suggested change
tweetTextList[index].style.font = "Lato,proxima-nova,Helvetica Neue,Arial,sans-serif;";
tweetTextList[index].style.fontFamily = "Lato, proxima-nova, Helvetica Neue, Arial, sans-serif";

Copy link
Member Author

@atharva-2001 atharva-2001 May 25, 2021

Choose a reason for hiding this comment

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

Oh, my bad, I fixed it now. Thanks again!
Could you please check this link to view the documentation?

@jaladh-singhal
Copy link
Member

@atharva-2001 the font type gets changed - thanks! But there are 2 scrollbars in right on embedded twitter feed - I'm not sure why the outermost container has the scrollbar now, it wasn't there earlier.

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.

@atharva-2001 the font type gets changed - thanks! But there are 2 scrollbars in right on embedded twitter feed - I'm not sure why the outermost container has the scrollbar now, it wasn't there earlier.

There's this one last thing (I know it has stretched alot, I always point out something!) :)

@atharva-2001
Copy link
Member Author

I'm sorry for getting it done so late, my bad I couldn't see the two scrollbars problem
It's fixed now though.

@jaladh-singhal jaladh-singhal merged commit 267906d into tardis-sn:master Jun 4, 2021
atharva-2001 added a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
…ardis-sn#1555)

* Replacing the contents of the news section with TARDIS twitter feed

* add css link

* removing two scrollbars

* [build docs]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Twitter feed on docs page to replace News section
3 participants