-
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
Sequence and visualizer switcher [cleaned] #379
Conversation
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.
An initial review.
@@ -65,18 +65,23 @@ | |||
Default styles should be for vertical mobile devices | |||
(devices narrower than --ns-breakpoint-mobile) | |||
|
|||
Small devices (landscape phones) | |||
@media (min-width: var(--ns-breakpoint-mobile)) { ... } | |||
// Small devices (landscape phones) |
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.
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.
Wow, ugh, yes, it does seem like this needs to be addressed before merging this PR. Not really my forte, so if anyone else wants to jump in I would welcome it, but if this PR ends up stuck on just this I will give it a shot.
OK, in addition to base performance/code review which still need to occur (and anyone feel free to pitch in), I feel as though there are a significant amount of other questions/concerns/aspects to consider and possibly improve/fix prior to merging this PR into ui2. Several that immediately come to mind are: [This first one needs some discussion of what design to pursue here.]
[So far, there is consensus that the following items should be changed as described in order to merge this PR.]
There are likely other things, very open to suggestions, and will start to process/analyze/make fixes based on Kate's feedback above. |
Regarding the switching icon, I agree it is too big. I may not be imagining your first suggestion in the way it was intended, but it seems like the user may not figure out how to switch in that scenario? Or does it look as if the title of the visualizer is inside a drop-down, which is an intuitive way to discover switching? I think the icon without text (text on mouseover) is pretty clear. I mean, if I was guessing what a big button next to the sequence or visualizer name with circle arrows on it was going to do, I'd guess it was something to the entire visualizer, so that's where I'd look for the capability to switch. This may need more thought or creativity, however, as I think discovering intuitively that switching is possible is crucial to the appeal of the tool. Some people are going to land there and just start playing to figure out what it is (even if we make a tutorial, I find I always click through those things without retaining anything). Maybe we could even make the switcher buttons dance until the user mouses over or uses one? Maybe that's a bad idea? I welcome more ideas! |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
I like the OEIS box in its new location. I guess in the longer term (beyond this PR) we might want OEIS sequences that the user adds to hang around in this dialog box for future use, with a delete button option. |
This comment was marked as outdated.
This comment was marked as outdated.
Yes, we shall see what the Delft team cooked up when we get to the next PR. I don't think we ever saw a demo of that. |
OK, gotcha. I can delete that branch at an appropriate time. Next option to try? Maybe make the name of the visualizer (or sequence) look like an entry area, the kind that pulls down to a little menu when you click on it? Vaguely like the one in the center of this image but in our case when you do click to drop down the options, it will be a whole panel of thumbnails? |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Implementing this feature involves abstracting the sections of the Gallery into a new SpecimensGallery component, and then using that component in the SwitcherModal. Note that in order to accommodate the desired captions of the SwitcherModal, the CardSpecimen interface used to specify the contents of a SpecimensGallery has to be generalized to allow arbitrary subtitles.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Feedback:
|
Nice call. I didn't even realize I had changed behavior. Before my latest commit, clicking anywhere below the Specimen Bar (with the menu and specimen name and play/pause button etc.) but not on the Switcher popup would close it. Currently, clicking below the Specimen Bar but not on the visualizer canvas closes the Switcher. It would be slightly tricky and I think slightly odd for clicking on the Specimen Bar to affect the Switcher -- the Switcher is more part of the content of the page, rather than anything to do with the header, and in fact, all of the buttons on the Specimen Bar still work while the Switcher is popped up (yes, you can rewind the current visualization and have it start playing again underneath the popup if you want). But I do think I know how I could also make clicking on the Specimen Bar close the Switcher (disabling all of the usual Specimen Bar interaction) if that's what we really want. So, when the Switcher is up, where, other than its close button, do we want a click to close it? Reasonable possibilities I can think of include: In (D), the Specimen Bar can't have its usual functionalities; in (A),(B),(C) presumably (and unless we do some extra work) the Specimen Bar would continue to operate exactly as normal while the Switcher is popped up. However, we could by doing that bit of extra work suspend operation of the Specimen Bar while the Switcher is up, even if clicking on it doesn't close the Switcher, if that's really what's most desirable. There may be other reasonable possibilities I am not thinking of, feel free to suggest. But I agree, the current behavior with my latest commit doesn't really make any sense and should be changed. I don't have a strong sense/intuition of which of these is best/most intuitive/most appropriate for our user interface (probably because it never really occurred to me to click anywhere except the close button if I was done with the popup and wanted to dismiss it). So I am happy to either discuss at today's meeting or just go with whichever you pick -- just let me know.
That's https://github.com/orgs/numberscope/discussions/6#discussioncomment-9970250, so out of scope for this PR (but happy to discuss/address it after the Delft PRs, perhaps along with human-readable URLs). |
To summarize the agenda on this PR from the meeting today, there are three remaining to-dos:
|
OK, that should take care of the first item. |
OK, actually I just got involved with it and have pushed a commit that I think changes Switcher invocation in all the ways we discussed. Please check it out and beat on it and let me know what you think. If I missed anything in the plan just let me know. |
It looks really good to me! And I think it does accomplish the "discoverability" really well, now that I see it in action. There are plenty of hints. I did beat on it a while and have no concerns except one small one: In the icon buttons on the header, the icons are slightly grey, not black. This should match (I think it's black right now). |
I matched the new buttons to the right of seq/vis names with the previously-existing buttons on the Specimen Bar: they are all dark grey, switching to black on hover, per the Delft team design. If you'd like different presentation/behavior, just let me know. Thanks! |
Weird, I'm just not seeing that on my screen in either Edge or Chromium. It is always-black. I verified it with gpick on a screenshot without mouseover. |
OK, I have to run now but when I next have a chance, I will compare Firefox (that I have been using) with Chromium. Edge isn't available for Linux, is it? |
I have Edge on Ubuntu. I opened Firefox, I don't see grey there either. |
Super weird. I will check all of FF, Chromium and Edge later and post screenshots. |
Ah, you are absolutely right. I must not have checked again after making the icon into a component. Should be OK now, let me know. |
I started taking a look at mobile because as far as I can see it is the only thing remaining on the to-do lists above, and despite what may have been said in the original submission of PR #359, at least in mobile emulation mode in Firefox developer tools, mobile still seems to be an absolute mess in this PR. To give the benefit of the doubt, maybe I messed up somewhere along in the cleaning/reintegration steps as the original PR sequence has now diverged quite substantially from the actual contents of ui2, and we really haven't been checking mobile for being at all reasonable along the way (since the original PR sequence didn't say mobile was "done" until #359). So my proposal is: @katestange and/or @Vectornaut, please do final checking/evaluation of this PR in all the usual ways, but continuing to ignore mobile. When one or both of you is satisfied everything is OK, please merge this into ui2. Then we will add to our post-checklist an item to beat on and discuss the mobile ui and fix it up to our satisfaction, presumably in the next PR to ui2 after the Delft ones and definitely before ui2->main. If this proposal is agreeable, then unless your checking reveals additional changes needed, which I am of course happy to work on, my plan will be not to do anything further on this PR and wait for it to be merged to ui2 (definitely squash!), at which point I will clean #360 and we will see how OEIS sequences work :-) etc. Thanks. |
Great! I think leaving mobile for later is a reasonable call. This PR is big and well-defined enough without it. I noticed that in workbench mode trying to change the visualizer results in a pop-up error. How do I test properly in production? |
I am 75% sure that is because the Grid visualizer is dead, and has been put in workbench (sort of hard to debug because of the way that our error popup blocks the javascript debugger). As far as I can see, you can just click "Ok" and everything will keep working except you can't use the Grid visualizer, and you can beat on everything else to your heart's content. |
Ok, I've been beating on this for a long time, so I'm satisfied. Would you like @Vectornaut to weigh in or would you like me to go ahead and squash/merge? |
I am personally comfortable with your squash-merging it to ui2. |
* feat: Sequence and Visualizer switchers, first pass at cleaned * fix: Always show the current background and stroke colors in Turtle * fix: Don't refresh empty default parameters * ui: move sequence search bar to right of Sequence Switcher popup title * feat: Switchers now show thumbnails of the modified visualizers Implementing this feature involves abstracting the sections of the Gallery into a new SpecimensGallery component, and then using that component in the SwitcherModal. Note that in order to accommodate the desired captions of the SwitcherModal, the CardSpecimen interface used to specify the contents of a SpecimensGallery has to be generalized to allow arbitrary subtitles. * fix: Equalize space between sections of Gallery * feat: Resize Switcher modal popups to fit specimen cards cleanly * feat: Placeholders for optional parameters * fix: Make placeholders lighter * ui: Pause visualizer while Switcher dialogues are open * ui: Position Switcher popup centered over visualizer canvas * fix: Restore Delft-submitted click behavior for closing Switcher * ui: Invoke Switchers via current seq/vis looking and acting like entry fields * fix: MageExchangeA icon should use current CSS color
By submitting this PR, I am indicating to the Numberscope maintainers that I have read and understood the contributing guidelines 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.
Switchers
* Adds a new button on each tab to change the sequence/visualizer. It is still not possible to switch to an OEIS sequence (that's added in the next PR in the sequence, but changing to all other sequences/visualizers should work, provided they work themselves.
Parameter Editor
* Updates the param editors to work with switching visualizers and specimens.
* More specifically, when a paramable is assigned to (like in the case of switching a visualizer specimen), the watch function gets called that makes sure to reset all the statuses of the parameters. This is so that no errors are carried over from the previous visualizer/sequence.
Specimen bar
* Refactors the buttons in the bar into a button container, and reworks the save button popup to fix some visual bugs.
Mobile
* Changes the mobile vs tablet-desktop breakpoint to 800px, as the specimen bar occupies a lot of horizontal space on the screen, and makes that breakpoint into a single global variable that can be changed in one place (the new src/assets/scss/_variables.scss file).
Scope