-
Notifications
You must be signed in to change notification settings - Fork 302
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
💻 New public adventures page #6092
Conversation
The syntax highlighter used to work by calling `initTranslations()` with the current level and language, which then manipulates global variables so that the syntax highlighter code has the right information to highlight the different levels and languages of Hedy correctly. To remove the need for global variables, we instead parameterize the Lezer parsers and pass the level and language into the generator at every time it is used.
for more information, see https://pre-commit.ci
…nto huijbers/no-global-trans
…nto huijbers/no-global-trans
for more information, see https://pre-commit.ci
javascriptPageOptions: js | ||
}); | ||
}, 1000); | ||
console.log('updateTSCode', e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably was added for testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, thanks
I am ready to ship it, but I think we might need to ask the POs to check if they are ok with the changes. @MarleenGilsing @AnneliesVlaar would you like to take a look? The UI has changed quite a bit. Also, cloned adventures are not automatically marked as public anymore. |
Btw when there are many adventures, I do not see a Next Page button in the bottom left corner (as on the screenshot). I just get a scrollbar: Screen.Recording.2025-01-13.at.14.44.02.mov |
Yes, this was on purpose. I don't think that cloning a public adventure to be automatically public as well is expected behavior. In the usual case, we would just have 2 public copies of the exact same adventure, which seems to not add any value. I'll put it in the PR body. |
Do you have more than 20? |
I think loading 5 adventures at a time is annoying for pagination (too few items), but having the list stretch the entire height of 20 items makes it way too long (I had that at some point but it doesn't work very well). A simple compromise is probably to put the NEXT PAGE button inside the scrollable container, and making it very obvious that there is scrolling to do? |
I did the things I said, hope this is better |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a strictly better version of the public adventures page both from a technical and UI perspective. I am approving this before the POs have reviewed this functionally because the PR has already caused merge conflicts. The feedback of the PO review will be addressed in a separate issue.
Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork). |
This pull request has been removed from the queue for the following reason: Pull request #6092 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (detail: Required status check "build" is in progress.) You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
I like the new design! It is more clear and I love the little icon in the list that says “This adventure has a solution example” <3 I do agree with Boryana that the scrolling through the list of adventures feels a bit weird. I do think the button within the scrolling container makes it better, although the button doesn't work for me, when I click on "next page" It shows "There are on public adventures matching these criteria. Try searching for something else." Maybe I don't understand the problem/solution of the scrolling container and how many items are loaded, but is it possible to change the "next page" button into a "show more adventures" button and when clicking that button show for example 10 adventures more (so 30 in total)? |
That's odd. Can you share your search criteria? And does it fail immediately after the first page?
Aha I see. You'd prefer an infinite-scroll type interface, instead of pagination? Yeah I can do that. |
I don't have specific search criteria, 'English', 'all levels', 'all tags' and indeed it fails immediately I have never seen a second page.
To infinity and beyond! |
This PR changes the UI of the teachers "public adventures" page. It does away with the tabs and replaces it with a scrollable list on the left side. It's also faster and doesn't rely on all requests coming in to a single Python process (which will not be true on Heroku).
Behavioral changes w.r.t the previous version:
print my boss is a poopy-head!
in the adventure), those changes are inadverently public It seems to make more sense that a user deciding to republish and adventure should be a conscious decision.This is what it currently looks like:
How to test
Log in as a teacher, and search and use the interface. Use clone, use flag, use delete if you are
teacher1
.