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

Update for 2024 #27

Closed
wants to merge 57 commits into from
Closed

Update for 2024 #27

wants to merge 57 commits into from

Conversation

jteijema
Copy link
Member

@jteijema jteijema commented Aug 26, 2024

Decided on a makeover. Let's automate the generation of the html files with a github workflow

@jteijema jteijema marked this pull request as draft August 26, 2024 14:24
@VeenDuco VeenDuco self-assigned this Aug 26, 2024
@VeenDuco
Copy link
Contributor

VeenDuco commented Sep 5, 2024

There are multiple things going on here within one pull-request:

  1. The original description; "Let's automate the generation of the html files with a github workflow"
  2. Changes to the text of the exercise ASReviewLAB
  3. Changing figures (and potentially their location)

It's not working well to review all these changes within one PR. Change 1 deserves a separate PR as originally described. It's a major overhaul (conceptually) that should not be mixed with other changes. I think it could also benefit from a bit more explanation in the readme on how it works and especially how people can now (more easily!) contribute. The changes to 2 could be easily merged. Changes to 3 have some discussion that holds up the review (the comment of Rens above seems like a way in which this can be done but also requires those figures to be updated when ASReview is updated, is that reasonable additional work to put on the developers of the frontend?).

It's not nice that for instance textual changes are hold up by an overhaul of the way the website is rendered. This can be avoided by pulling apart these pull-request. @jteijema please create 3 separate pull-requests would allow for better review and discussion.

@jteijema
Copy link
Member Author

jteijema commented Sep 5, 2024

1 and 3 are inherently linked; the changing of figures and their location is managed differently because of the changing to the generation of the html files (i.e. the workflow manages image location).

I agree that we can move the changes for point 2 to a different PR. We needed the updated site for the latest summer school and we wanted to avoid pointing the users to even more websites.

@VeenDuco
Copy link
Contributor

VeenDuco commented Sep 5, 2024

Alright, then let's try to make it work for now. Let's try the following solution:

  • We keep all images related to the tutorials in ASReview-Acadamy. We don't want to have to update files in the main repo of asreview if we need changes here. Besides that, educational screenshots (including arrows pointing to part of the figure) are not the same as needed for documentation.
  • To make sure it's clear from what version everything is, we will update the tutorials to mention this at the top and rename screenshots (when updating them) to include version numbering; e.g. step_1_V1.6

If we do this, control and responsibility for the correctness of the screenshots is fully in this repo itself. That takes away any need for asreview main repo maintainers to think about these screenshots when they update. We will follow their work when updating tutorials.

@Rensvandeschoot
Copy link
Member

Figures and content have been solved in #28

The GitHub actions should be a new PR.

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.

4 participants