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

Automatically update now and next. #145

Merged
merged 37 commits into from
Apr 15, 2019
Merged

Conversation

mstratford
Copy link
Member

@mstratford mstratford commented Jun 19, 2018

Improvements still to make:

  • Update status of message box.
  • Update just after the hour and half hour, not just every 30mins.

@mstratford
Copy link
Member Author

Closes #41

@Danzibob
Copy link
Member

Danzibob commented Jun 21, 2018

There's still a couple of complexity issues (which seem entirely valid but I don't know what to do about them) and one "generic item sink" which I think is a non-issue. Otherwise, ready for review

Was also considering adding a bar in the style (M|T|W|T|F|S|S) above / next to the jump button to link to each day, to further enhance mobile-view. Thoughts?

I thought this button would be far more tied into the C&N updater this PR was for, but it turned out not to be at all. Sorry Matt :p

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

codacy's complexity threshold can probably be raised - we use 10 at work

Otherwise fine - just check your coding style - lots of missing spaces before { and after ,

public/js/schedule.js Outdated Show resolved Hide resolved
@mstratford
Copy link
Member Author

mstratford commented Jun 21, 2018 via email

@Danzibob
Copy link
Member

Danzibob commented Jun 26, 2018

With regards to the API key, I'm inclined to agree with @LordAro. As is, the API key exposed in the JS is the same as that in .myradio.key, and it's only being used for a single endpoint. This notation was used elsewhere, but has since been removed after messages were moved back-end.

I'm starting a new branch to implement this change, and ideally we can merge that before this.
EDIT: Matt's already inserting a seperate key from config.toml, all I have to do is mess about with making a new key for public JS

Added new API key to database with required permissions
(required permissions being literally none)
views/schedule_week.tmpl Outdated Show resolved Hide resolved
views/schedule_week.tmpl Outdated Show resolved Hide resolved
Copy link
Member

@Brookke Brookke left a comment

Choose a reason for hiding this comment

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

If it works then looks good to me other than a few erroneous lines/missing lines

Copy link
Member Author

@mstratford mstratford left a comment

Choose a reason for hiding this comment

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

Seems good, seems to work changing my computer clock around.

config.toml.example Show resolved Hide resolved
public/js/index.js Outdated Show resolved Hide resolved
views/elements/current_and_next.tmpl Show resolved Hide resolved
@mstratford
Copy link
Member Author

Note #184 changes some of the text layout for the now and next.

@mstratford
Copy link
Member Author

Right, OK. I've fixed all the Codacy issues (bar the Complexity, which was recommended to change to 10 in the settings). Plz merge ❤️

@Brookke Brookke merged commit 878c41d into development Apr 15, 2019
@mstratford mstratford deleted the mstratford-updating-nowNext branch November 1, 2019 01:08
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.

None yet

4 participants