-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Version switcher #3334
base: develop
Are you sure you want to change the base?
Version switcher #3334
Conversation
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. |
I made some updates to put the version settings in a modal! To do that, I mostly had to switch the tabs to being controlled instead of uncontrolled so that I could make an action that sets the starting tab. Screen.Recording.2025-02-19.at.8.18.53.PM.movOne thing that isn't quite handled yet is multiple p5.sound versions, that might take a bit more thought. |
I looked into a few HTML parsing options using libraries, and.... none of them did a better job than the native DOMParser API at preserving whitespace (meaning: it doesn't do a great job.) So for now I'm just going to keep using the DOMParser API, and possibly if we want we can make it also run Tidy Code afterwards. |
Another update! Now I'm detecting p5.sound versions, and offering a button to revert back if it was something non-default and we change it at all. This is stored in React state for the IDE view, so even if you close and open the preferences modal, the revert button will still be available. Screen.Recording.2025-02-27.at.6.07.01.PM.mov |
Hi @davepagurek , just checked this out with @raclim ! What do think if the big button is not a button but an explanation of what happens if you click "on?" Right now looks like the big button reverts but the "On" button will update to a new version, which is a little unintuitive, so we were thinking about how to make it a bit more understandable. What if there are still only "on" and "off" as clickable buttons, but in this special state, there's a (pink, obvious, bold?) text like "Want to turn this addon back on? The editor will use the original version you were using before turning it off!"
Sounds like this means if the user closes the tab and reopens, the information is lost? That seems fine for me, since then the basic idea of "go back to what you had before" is also very hard to remember and interpret. |
Agreed, that sounds like more intuitive behaviour. It does mean that old sketches wouldn't have an easy way to switch to using the new p5.sound.js, is that OK? I think that leads to the least confusion for now, and maybe in the future we could add a button like "Try the new p5.sound addon" once we or a contributor has the time to consider the UX edge cases.
Right, exactly. |
Here's an initial little animation to play when 2.0 is selected! Screen.Recording.2025-03-11.at.1.27.15.PM.mov |
Ok the p5.sound toggle now looks like this if you turned off a custom version, and toggling it back on restores it: I've also made some UI updates to allow the long dropdown of versions to scroll, and detect p5.sound.min.js along with just .js. Still to do before this is shippable:
|
That looks great @davepagurek !
Reasonable!
I think this should is linking to the 2.0 beta reference website and the "Switching to 2.0" tutorial which is WIP for now |
@davepagurek I love the animation, it's so cute!!!
Thanks for mentioning the admonitions in the figma! It was mostly added as a suggestion—I forgot that I wanted to ask if you and @ksen0 had thoughts on it? |
I like them! do you know if we have them anywhere else in the web editor yet or if it would be a new component? Also do you know if we have a button with a down arrow resembling a |
The closest thing that we have to it so far is toast messaging, but I feel like it will probably be a new component! I don't think we have anything like that yet either 😅 I think the button doesn't have to resemble the one in the design too much though! |
@@ -254,6 +257,9 @@ class Editor extends React.Component { | |||
if (!prevProps.unsavedChanges) { | |||
setTimeout(() => this.props.setUnsavedChanges(false), 400); | |||
} | |||
} else if (this.getContent().content !== this.props.file.content) { | |||
// TODO: make this not break regular edits! |
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.
@davepagurek This was pending the CM6 upgrade which will need ~2-3 weeks (after p5.js 2.0 release in any case). So we can move forward without waiting to resolve this TODO but need to keep it in mind when the upgrade is happening, @raclim will coordinate!
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.
I think I made this not break on the current version of CM so I think this TODO comment may be good to remove. I'll double-check this week!
This is looking great! Minor comments below. (1) Admonition - the functionality is great but Rachel and I were discussing color and title choice, given one of the original design intents:
How about:
The copy itself looks great. (2) the "TODO add helpful info..." - how about:
It's kind of kicking the can down the road (for me to add transition instructions to the p5.js-compatibility repo) but I think if we have ONE link, it feels like it should be the compatibility add-ons link in particular. If there's a better link, like a transition tutorial, we can update this copy, so it doesn't need to be blocking. So with the above, if it sounds alright to you, I think we can call this one done! |
That all sounds good to me! I'll make those updates this week. I assume then we'll leave this PR open until the 2.0 release and then merge after that? |
@davepagurek Yes, that sounds great! I can give approval whenever it's done and we can just coordinate on merge timing. |
Here's what it looks like now! I saw in the figma a blue variant, which I used here because it felt like it had a little more contrast than another shade of grey, but I can also try a grey variant if you prefer. Also @raclim I wasn't sure if there was a precedent yet for translatable text that has links within it so I ended up putting some markdown in the translations file and using the existing |
It also occurs to me that before actually updating (and also after each release), we'll want to update the list of p5 versions on npm here: p5.js-web-editor/client/modules/IDE/hooks/useP5Version.jsx Lines 9 to 14 in 53a517b
This will work for now but let me know if there's any way I can make it easier to integrate this into the release workflow for the future! Also one last small blocking update: once we make a public release of the p5.js-compatibility repo, I need to update the URLs for that. |
Hi all! The design is looking good! I like how it integrates with the existing modal for settings, while also providing an extra cue above the editor for discoverability. The copy and aesthetics also look great (not to mention the fun animation Dave added!). Below, I'll outline a few possible accessibility and usability improvements. (I'm partly going off of the screen recordings and captures, so it's possible some of these points have already been addressed.) Color contrast (accessibility)Have you run the new colors through a contrast checker for accessibility? For example, it looks like the colors (background: Version-number button (accessibility, discoverability)Hover behavior, persistent visual cueTo the left of the version number, the text "sketch.js" isn't interactive. To the right, the "Preview" text isn't interactive. The version number is interactive, but there's no visual distinction between it and those neighboring elements. There are two things we might add to the version number, both of which would improve consistency with existing features and therefore guide user expectation:
An extra visual indicator would be in line with the WCAG recommendation to not rely on color alone to distinguish a visual element, and it'd indicate interactivity without requiring the user to hover with a mouse. For this purpose, similar features currently use a dropdown icon, a pencil icon, or a checkbox. In this case, how about a small version of the gear icon, to indicate that this feature will open the settings modal? (There doesn't seem to be a standard icon for buttons that open a modal. An ellipsis does seem to be a standard way to indicate that further action is needed—according to WAI and UX Stack Exchange—but that might be problematic here. Specifically, an ellipsis might be confusing after a version number that already uses periods as separators; it's also not an icon, so it'd be inconsistent with the other features.) PositioningI suspect there will be greater user engagement if we move the version number to the right of the auto-refresh and project-title features. I'm not sure if there are any significant cons to this, but here are a couple of pros:
Putting it on the right would also maintain the placement of the existing features, so that the interface would conform to the expectations of current users. Explanatory text for p5.sound.js add-on (accessibility, semantics)Could we wrap the radio group in a More generally, it looks like a good idea to replace the I'm not sure, but I think you can get the correct positioning by replacing the legend {
font-weight: bold;
float: right;
margin-bottom: .5em;
} In case you run into any other issues, there's a breakdown of special styling considerations for Compatibility add-onsTwo questions:
Bug?I noticed that it's possible to tab to the settings button and open it with Enter, but it looks like it's not possible to tab through the options in the modal. When I tried to do that, tabbing moved me to the sidebar button and then the editor. I'm guessing this would make the features inside the modal (including the new versioning feature) inaccessible to keyboard users? What do you think, @raclim? If this is a real issue, maybe we could open a separate PR to deal with this. Minor polishPlease forgive me for being me here... If you all prefer to ignore the following points, I would understand 😜
Footnotes
|
Thanks @GregStanton! I pushed some updates:
![]() ![]() So far I haven't changed the position of the version switcher yet, or changed to "add-on" (that's a quick change, we just need to also update the p5.js-compatibility repo to match -- also, one other data point, we refer to addons as "libraries" on the p5 site) -- let me know what you think about those @ksen0! None of them are difficult changes if we want to do them, just want to make sure we've got consensus. |
Hi @GregStanton and @davepagurek , thanks so much for this really thorough review and update! Here's another co-authored review from @raclim and I. Terminology: "add-on library"Addon vs add-on: I've only ever see "addon" as a term in the p5.js ecosystem, and it seems to be used, e.g., in contributor documentation, which call them "addon libraries" as yet another data point. Using either add-on or library would be an update from prior language, and neither Rachel nor I have attachments about it. On discussion, we would suggest "add-on library" which is the corrected spelling of an already common phrase. "Library"
"Add-on"
Compromise (yes, and): "Add-on Library?"
Compatibility
|
Some initial thoughts on this design:
Another idea could be to keep the positioning where it is, but add some kind of notification dot or other indicator (slowly pulsing the color of the text?) the first time a user sees the version picker that goes away permanently once they click it. That could let people know that it exists, but still keeps it sort of out of the way after that. For clickability, with an icon + hover state, it already matches the clickability of the editable sketch title above, so I think that could be sufficient if we don't also need more visibility.
I think the ideal behaviour is like, when you switch to 1.x, it hides the compatibility addon toggles and and also turns them off, but also it remembers what you had so that if you switch back to 2.x, you return to the same state as before. So far I've left them visible all the time so that at least you can manually enable/disable. I think nothing blocks implementing this other than taking the time to do it, which is a little less easy than just conditionally hiding as it adds some state management, but I'll try to get to that this week.
So far it's manual. A while ago I started looking into getting version info from npm for the libraries page of the p5 site, so it's definitely possible, but it's unclear what the API rate limits are, so ideally this would be something we could fetch only periodically (e.g. just when we deploy a new version?) instead of doing it live for every visitor. It'd be great to be able to do this with the p5 version list too. @raclim do you know if there's anything existing in the setup so far to run and cache a value once on deploy? Alternatively, we could put those values in a special file that gets auto-generated via a script that we manually run sometimes. In any case, we can launch without this, but we could reduce the amount of maintenance in the future with this. |
I think that makes sense conceptually, I don't mind keeping it where it is. @GregStanton do you see any accessibility concerns about the dot concept here?
This behavior can also be an issue and a future work separate from this PR.
What about an action to pull versions every month or so (can also be triggered manually) and that creates a PR with the updated lists? Also a fine issue for the future and out of scope for this PR. |
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.
Overall, there are some outstanding points that can be follow-up issues, but it looks great and just about ready to go when the new release is ready.
Here's an experiment to see what a version picker could look like on the web editor.
What works so far:
index.html
is edited seems stable and not super laggy!Screen.Recording.2025-02-06.at.8.24.27.PM.mov
What does not work so far, if I uncomment the code to actually replace the contents:
dom.documentElement.outerHTML
doesn't have quite the same formatting as before (e.g. theDOCTYPE
tag is gone) -- maybe there's another attribute I can use, need to experiment moreScreen.Recording.2025-02-06.at.8.17.15.PM.mov