Initial editor tabs implementation with CAD package guides #519
Reference in New Issue
Block a user
Delete Branch "editor-tabs"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
https://user-images.githubusercontent.com/23481541/133914435-d9443aba-79f2-41d7-a48c-da8a8ecac9ba.mp4
Implemented
Not implemented
Other Work
raw-loaderto Webpack to allow the following file types to be imported as raw text:.SCAD.py.jscad.mduseIdeStateusing this to break outinitialCodeanduserGuidefiles for each CAD package within their own directories.CadPackagecomponent@@ -0,0 +50,4 @@{matches[1]}</a>)}Overengineered? Definitely. But I really wanted to get links into the meta fields for this and for people to be able to edit them easily using the standard YAML frontmatter in GitHub-flavored Markdown. So this checks if the metadata value is a Markdown-style link.
I wasn't sure how to handle the INIT type but I didn't want those red squiggles anymore.
This should probably be broken out into a new component soon. I can do it now if you think I should @Irev-Dev.
@@ -272,0 +229,4 @@...state.models.slice(payload + 1),],currentModel: payload === 0 ? 0 : payload - 1,}These are not used anywhere yet, so I haven't gotten to test them out.
@@ -3,3 +2,4 @@import { cadPackages, initCodeMap, initGuideMap } from 'src/helpers/cadPackages'import type { RootState } from '@react-three/fiber'import type {RawCustomizerParams,These work but cause TS errors in VS Code. Unsure how to fix but will investigate, I have a hunch that
.d.tsfiles might help this?@@ -3,3 +2,4 @@import { cadPackages, initCodeMap, initGuideMap } from 'src/helpers/cadPackages'import type { RootState } from '@react-three/fiber'import type {RawCustomizerParams,Got it fixed with
globals.d.tsfileI made a couple small suggestions, so I won't merge, but it's approved so merge when you're ready :)
As is my style these days I need make an extra commit that you can pull in if you like
bb4659a2dd(there's a comment there too with a bit of an explanation)This is a really nice solution.
@@ -9,0 +11,4 @@openscad: {label: 'OpenSCAD',buttonClasses: 'bg-yellow-800',dotClasses: 'bg-yellow-200',😁
I don't think we need to do that now, its a 90 line file, not screaming to be broken down yet.
Maybe if tabs get more complex.
Just thinking ahead here and leaving some thoughts;
When changing tabs between code tabs, I hope there won't be any trouble just updating state (
valueandlanguagein particular). There is a "recommended" way of doing tabs with monaco, but don't think it really applies to@monaco-editor/reactI think Torsten is the current maintainer, but am not sure. Probably a good starting point I guess ideally these files would be maintain by a person from each project.
I should add a
.mdfile insrc/helpers/cadPackagesthats an explanation of how the integrations work, as there are more parts to it now as well.Do you think that moving this map as well as
initCodeMaptosrc/helpers/cadPackages/index.tsthen importing into this file would make sense?Would clean up the imports in this file and keeps the cad package config together more?
@@ -272,0 +229,4 @@...state.models.slice(payload + 1),],currentModel: payload === 0 ? 0 : payload - 1,}Could we maybe comment these out then?
I think that a good compromise between "we're going to use them very soon so lets leave them" and "Lets not add dead code to the project".
Maybe this falls into the bucket of preemptive optimisations but I think it's okay since its not adding much more complexity or new patterns. Basically I'm not sure how expensive regular expressions can be to run over and over so might be best to play it safe
Hey, removing approval because the user profile seems to be broken on this branch.
getting:
Since there are changes to that file I'm guessing it's causing an issue
Ah of course I hadn't thought of this rerendering a bunch of times! Thank you seems very helpful.
Sweet I can ask him in Discord about the maintainer things, Marius was justed on the main site still.
Oh yeah I definitely want to do that Monaco best practice if I understand you correctly! I named the field
modelsso that when the time comes we can add each code tab as true models with their own language and current state settings. I thinkcodeprop onIdeStatewill need to change or merge withmodelswhen we tackle multi-fileYeah that does sound good I'll give it a shot
Ah geez thanks for catching that, I should have realized that the
CadPackagebutton is used in several places and needed broader testing. I'm pretty sure it's related to theonClickprop I added but I'll check it out.Yeah sounds good, I'm thinking whenever we need to add drag and drop reordering of tabs.
@@ -272,0 +229,4 @@...state.models.slice(payload + 1),],currentModel: payload === 0 ? 0 : payload - 1,}Of course, good call.
@@ -8,6 +8,7 @@ export const QUERY = gql`idtitlemainImagecadPackage@Irev-Dev I didn't catch that we didn't have the cadPackage badges showing on the UserProfile before!
@@ -8,6 +8,7 @@ export const QUERY = gql`idtitlemainImagecadPackageIt wasn't related to
onClickbecause I made that optional, but rather the reworking to make it a config withdotClassesandbuttonClassesbased on the current CAD Package which was undefined inUserProfilebecause of this missing query field.@@ -8,6 +8,7 @@ export const QUERY = gql`idtitlemainImagecadPackageAh nice.
Sorry I haven't been able to find everything in one go, but I noticed that cadPackage badge thing on the project profile now has a pointer but clicking it does nothing, which is not the case in main.
https://user-images.githubusercontent.com/29681384/133941682-c5f999dd-20c8-4812-bfde-57dc5264c893.mov
Could we either remove the pointer or make clicking do something? I'm not sure which is better.
mini settings button is confusing me
I can click it to open settings, but can not use it to close settings
had to look all the way down left to see the big button for settings and use that to close the settings
It's somewhat intentional (in that it's a compromise we're okay with for now), we'll keep it in mind. (#507)
@Irev-Dev Ah yes can do, I had removed the hover background opacity change but didn't remove the cursor pointer setting.
Ah @hrgdavor yeah some control tweaks will probably come later and animations will help make that relationship more clear.
@@ -17,3 +42,4 @@className = '',dotClass = 'w-5 h-5',onClick,}: CadPackageProps) => {@Irev-Dev I thought about simply leaving the button and removing the
cursor: pointerif noonClickis provided, but I think this is more semantic and will prevent some future "button cannot be child of button" warnings in future.@@ -17,3 +42,4 @@className = '',dotClass = 'w-5 h-5',onClick,}: CadPackageProps) => {Looks good, dynamic tag is also an option
https://stackoverflow.com/questions/33471880/dynamic-tag-name-in-jsx-and-react
And @Irev-Dev you don't have to be sorry, I am. I should be doing this work before you review. I think I'm starting to understand how to assess where to test for regressions, looking for other places where a component I'm working on is used in other contexts and such.
@@ -17,3 +42,4 @@className = '',dotClass = 'w-5 h-5',onClick,}: CadPackageProps) => {Oh nice this is more the stuff I was looking for! In this case I'm cool with the way I did it because I also added the class name, but I'll keep this in mind in future yeah?
@@ -17,3 +42,4 @@className = '',dotClass = 'w-5 h-5',onClick,}: CadPackageProps) => {Yup, thats what I was going for, just an FYI if you weren't aware of it 😁
Yay, thanks for bearing with me.
I did find one more bug, but it's a trivial and amusing one, will get clean up with more tab work.
https://user-images.githubusercontent.com/29681384/133971317-598bdb14-7929-48fa-95ee-9cf85b2850ca.mov