-
Notifications
You must be signed in to change notification settings - Fork 15
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
Gallery page #341
Gallery page #341
Conversation
Restructuring
Default implementation of aspect ratio in P5 visualizer template
Merge code restructuring into main
Applied requested PR changes
As per plan, we want to merge into branch |
The branch has been adjusted to be compatible with ui2. It is now directed to merge with ui2 instead of main. Apologies for the oversight. |
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.
OK, that's all the review I could do from the code alone. It won't run for me at the moment (see the comments about "sass"). Once these are taken care of and it's running, I (and/or other members of the Numberscope team) can complete the review. Thanks!
Almost ready to finalize the review, just one remaining conversation about the "Change Visualizer" button. Also, just for your information and the rest of your team, we use the convention that only the initiator of a conversation should mark it as resolved (not the respondent) -- not sure if there is a way for us to enforce that in the GitHub user interface. So just as a courtesy, please let us the PR reviewers judge whether a conversation is resolved. Thanks! |
My apologies I didn't know about this convention. I'll leave things unresolved from now on. |
OK, this branch now passes typecheck and runs and the dev server is responsive. I observed the following "unfinished" or "in progress" aspects to the state of the frontscope UI as of this PR. Please just confirm that you are comfortable making a commit with these aspects, and I will go ahead and merge when I have your confirmation:
If you are OK with making a commit in this state, we will go ahead and do so. Thanks for confirming. |
We were aware of these shortcomings and after having discussed it with the team we are comfortable with making a commit in this state. All of these issues and other features will be addressed in future commits. |
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.
All comments have been addressed. Ready to merge.
Adds a route that shows a panel of thumbnails of multiple different specimens. This can be used, for example, for a curated list of interesting visualizers, or for an individual user's own visualizers. --------- Co-authored-by: Max Derbenwick <[email protected]> Co-authored-by: maxd0328 <[email protected]> Co-authored-by: Kaldis Berzins <[email protected]> Co-authored-by: mbavec <[email protected]>
By submitting this PR, I am indicating to the Numberscope maintainers that I have read and understood the [contributing guidelines] (https://numberscope.colorado.edu/doc/CONTRIBUTING/) and that this PR follows those guidelines to the best of my knowledge. I have also read the pull request checklist and followed the instructions therein.
This adds the CSS for the gallery screen and added basic css variables to the app.vue file. Some of the relevant code was also refactored to make names more descriptive and compatible with future developments. Additionally, added an icon font and removed bootstrap.