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

Undo renaming of "smfAnnouncements" div to avoid breaking the news #5264

Merged
merged 3 commits into from Dec 25, 2018

Conversation

Projects
None yet
5 participants
@Oldiesmann
Copy link
Member

Oldiesmann commented Dec 24, 2018

Signed-off-by: Michael Eshom oldiesmann@oldiesmann.us

Undo renaming of "smfAnnouncements" div to avoid breaking the news
Signed-off-by: Michael Eshom <oldiesmann@oldiesmann.us>
@albertlast

This comment has been minimized.

Copy link
Collaborator

albertlast commented Dec 24, 2018

i "forced" oldy to give a proper describtion,
here it is:

The javascript to display the SMF news in the admin center expects the div to have an ID of "smfAnnouncements". By default that div displays a text string saying "You are unable to connect to simplemachines.org's latest news file.". The JavaScript replaces that text with the appropriate news that it gets from simplemachines.org
So if the div is named "smf_news" instead of "smfAnnouncements", the JavaScript won't work

@Gwenwyfar

This comment has been minimized.

Copy link
Member

Gwenwyfar commented Dec 24, 2018

And this is why I hate doing changes without testing everything first... but oh well.

The javascript was also renamed, are you sure this fixes it?

https://github.com/SimpleMachines/SMF2.1/pull/5264/files#diff-b67fb0c40e61a708da98389c4692c021L124

Edit: Just tested it and it doesn't. Must be something else in the javascript.

@Oldiesmann

This comment has been minimized.

Copy link
Member

Oldiesmann commented Dec 24, 2018

I'm not sure what that change does exactly, but the JavaScript to actually display the news items is pulled directly from simplemachines.org, so we'd have to change it on that end for this renaming to work. That JS expects it to be smfAnnouncements.

Replace a few more instances of smf_news with smfAnnouncements
Signed-off-by: Michael Eshom <oldiesmann@oldiesmann.us>
@Oldiesmann

This comment has been minimized.

Copy link
Member

Oldiesmann commented Dec 24, 2018

Didn't see the comment about the JS being renamed until @SychO9 pointed out the changes... Latest commit should take care of those as well

And a couple more...
Signed-off-by: Michael Eshom <oldiesmann@oldiesmann.us>

@Sesquipedalian Sesquipedalian merged commit 6599074 into SimpleMachines:release-2.1 Dec 25, 2018

2 checks passed

Scrutinizer Analysis: No new issues – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Oldiesmann Oldiesmann deleted the Oldiesmann:fix_news branch Dec 26, 2018

@jdarwood007

This comment has been minimized.

Copy link
Member

jdarwood007 commented Dec 28, 2018

Note, its not that we can't change it, but this change has to go through approval from the lead developer and Site team (Me). My handling will be needing to update the javascript code to both support all editions properly. Typically this is something we should do early on in the development process.

@Gwenwyfar Gwenwyfar added the Regression label Jan 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment