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

Detailed display #22

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Detailed display #22

wants to merge 12 commits into from

Conversation

janmn
Copy link
Collaborator

@janmn janmn commented Jun 28, 2024

Created an additional field to display the remaining time until the start of the next event if idle or the time until the end of the current recording if capturing.

An additional page for the backend was created which contains all planned events for the next 24 hours from the time of the request. The information for each event is the title, the start time (in ms) and the end time (also in ms).

@janmn janmn requested a review from lkiesow June 28, 2024 10:04
@janmn
Copy link
Collaborator Author

janmn commented Jun 28, 2024

This fixes #21

Copy link
Member

@lkiesow lkiesow left a comment

Choose a reason for hiding this comment

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

Mostly nitpicks. But parsing JSON using regular expressions is… interesting 🙈

However, If I have a planned event (here it's starting in ~17 min) and start an ad-hoc recording before that, the display shows that the event ends when the next scheduled event ends. That's not correct. I'ts certainly a special case. Maybe we should just show nothing if there is a non-scheduled recording?

Screenshot from 2024-07-15 22-41-43

assets/index.js Outdated
@@ -41,7 +47,53 @@ function updateTimer() {
}
return response.json()
}).then(capturing => {
console.debug('capturing', capturing)
console.log('capturing', capturing);
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the debug logging and turn this into a global log? I'ts often suggested to not use console.log at all.

assets/index.js Outdated
Comment on lines 51 to 54
if(is_active != capturing && !capturing){
updateCalendar()
}
is_active = capturing;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why we do this additional update? We do an update every minute anyway, don't we?

assets/index.js Outdated Show resolved Hide resolved
assets/index.js Outdated

now = Date.now();
if (calendar.length > 0){
t = is_active ? calendar[0].End : calendar[0].Start;
Copy link
Member

Choose a reason for hiding this comment

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

You don't use t anywhere else, so make it a local variable or constant (since it's never being modified). Also, t isn't the greatest name for a variable ;-P
Maybe something like

Suggested change
t = is_active ? calendar[0].End : calendar[0].Start;
const event_time = is_active ? calendar[0].End : calendar[0].Start;

assets/index.js Outdated
Comment on lines 70 to 71
console.debug('Calendar is empty');

Copy link
Member

Choose a reason for hiding this comment

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

You could probably already return here.

main.go Outdated
Comment on lines 47 to 49
Title string
Start int
End int
Copy link
Member

Choose a reason for hiding this comment

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

We use lowercase for the other JSON endpoints. Maybe we should do that here as well?

main.go Outdated
r.GET("/calendar", func(c *gin.Context) {
client := &http.Client{}
// Cutoff is set to 24 hours from now
cutoff := time.Now().UnixMilli() + int64(86400000)
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need the explicit conversion to an int64 here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, we actually don't :D

main.go Outdated
Comment on lines 174 to 181
s := string([]byte(bodyText))
start := regexp.MustCompile(`"startDate":[\d]+`)
end := regexp.MustCompile(`"endDate":[\d]+`)
t := regexp.MustCompile(`"event.title":"[^"]+"`)

startDates := start.FindAllString(s, -1)
endDates := end.FindAllString(s, -1)
titles := t.FindAllString(s, -1)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we use regular expressions to extract data from the JSON and not just parse the JSON?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because the JSON contains some characters that aren't parsed correctly and that caused errors

right: 0;
padding: 3vw;
font-size: 3vw;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm really not sure, but I'm wondering if this would look better with a light font. Maybe something like

Suggested change
}
font-weight: 400;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lighter font looks good, but 400 looks too thin. Using 500 for the info field.

assets/style.css Outdated
align-items: center;
position: absolute;
top: 0;
bottom: 75;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you want this element to be positioned 75 (unit is missing) from the bottom of the container. That's what CSS bottom does. You probably just want to remove this.

Suggested change
bottom: 75;

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.

2 participants