h3n3/cadhub
1
0
Fork 0
Code Issues 35 Pull Requests Packages Projects Releases Wiki Activity

Sidebar tray #507

Merged
Irev-Dev merged 12 commits from sidebar-tray into main 2021-09-18 08:22:13 +02:00
Conversation 11 Commits 12 Files Changed 14 +513 -176
Irev-Dev commented 2021-09-12 09:18:53 +02:00 (Migrated from github.com)
Copy Link

Sorry creating a WIP PR, as it make it easier to make inline comments, otherwise they need to be on individual commits.

Sorry creating a WIP PR, as it make it easier to make inline comments, otherwise they need to be on individual commits.
👍 1
Irev-Dev (Migrated from github.com) reviewed 2021-09-12 09:21:17 +02:00
app/web/src/index.css
Irev-Dev (Migrated from github.com) commented 2021-09-12 09:21:17 +02:00
Copy Link

The sr-only class in tailwind looks pretty similar, definitely not the same though.
https://tailwindcss.com/docs/screen-readers

The sr-only class in tailwind looks pretty similar, definitely not the same though. https://tailwindcss.com/docs/screen-readers
Irev-Dev (Migrated from github.com) reviewed 2021-09-12 12:03:13 +02:00
app/web/src/components/IdeSideBar/sidebarConfig.tsx
Irev-Dev (Migrated from github.com) commented 2021-09-12 12:03:12 +02:00
Copy Link

This open param looks like it wasn't used, so not sure what the plan was with it here, but it smells a little to me, in that this data structure contains with copy/user info, and app state and I think it will be hard to manage like this.

I've made a commit with a demo of the tray being controlled by the app data store, which I think is probably the right approach, even thinking about it abstractly

a button anywhere on the page can open the tray

indicates to me this should be handled through app state

This open param looks like it wasn't used, so not sure what the plan was with it here, but it smells a little to me, in that this data structure contains with copy/user info, and app state and I think it will be hard to manage like this. I've made a commit with a demo of the tray being controlled by the app data store, which I think is probably the right approach, even thinking about it abstractly > a button anywhere on the page can open the tray indicates to me this should be handled through app state
Irev-Dev commented 2021-09-12 12:33:15 +02:00 (Migrated from github.com)
Copy Link

Okay, take a look at 69c83d33b1 for a bit of a proposal. I hope I'm being helpful.

Okay, take a look at https://github.com/Irev-Dev/cadhub/commit/69c83d33b1bb6c2349787983edf37aa55db1f0ae?branch=69c83d33b1bb6c2349787983edf37aa55db1f0ae&diff=split for a bit of a proposal. I hope I'm being helpful.
👍 1
franknoirot (Migrated from github.com) reviewed 2021-09-12 18:44:08 +02:00
app/web/src/components/IdeSideBar/sidebarConfig.tsx
franknoirot (Migrated from github.com) commented 2021-09-12 18:44:08 +02:00
Copy Link

Love this thank you I will take a look

Love this thank you I will take a look
franknoirot commented 2021-09-12 18:57:32 +02:00 (Migrated from github.com)
Copy Link

Yo @Irev-Dev your proposal is so much simpler it's a breath of fresh air. I'll read through it better in a little bit but I definitely think that's the way to go, sorry for leading us astray I hope my approach at least served as a rubber duck for you 🤦🏻‍♂️. Should we merge your proposal over mine, and then I can do the semantic HTML and CSS fiddling you were talking about, or close mine and replace with yours to do that? Not sure how to wrangle the branches.

The "only one sibling setting open at a time" constraint sounds like a feature as much as a bug, and I don't think it will throw off most users, other than potentially ones with screenreaders but we're gonna need wayy more engineering help than just you and I to get accessibility down on a 3D modeling code editor app 😅.

Yo @Irev-Dev your proposal is so much simpler it's a breath of fresh air. I'll read through it better in a little bit but I definitely think that's the way to go, sorry for leading us astray I hope my approach at least served as a rubber duck for you 🤦🏻‍♂️. Should we merge your proposal over mine, and then I can do the semantic HTML and CSS fiddling you were talking about, or close mine and replace with yours to do that? Not sure how to wrangle the branches. The "only one sibling setting open at a time" constraint sounds like a feature as much as a bug, and I don't think it will throw off most users, other than potentially ones with screenreaders but we're gonna need **wayy** more engineering help than just you and I to get accessibility down on a 3D modeling code editor app 😅.
franknoirot (Migrated from github.com) reviewed 2021-09-12 21:35:47 +02:00
app/web/src/index.css
franknoirot (Migrated from github.com) commented 2021-09-12 21:35:47 +02:00
Copy Link

Ah should've know they'd have a utility for that

Ah should've know they'd have a utility for that
Irev-Dev commented 2021-09-12 22:03:38 +02:00 (Migrated from github.com)
Copy Link

Cool glad you like it :)

In terms of branches @franknoirot , I think you should have a go at the wrangling them, that's how you'll get comfortable doing it. Worst thing is you break both you the branches locally in which case you can delete them locally and pull them again from github for another attempt. If you're nervous about it you make a new branch off sidebar-tray as a dummy to test the sequence, and if it works you can do it for real on your original branch.

This is pretty the same situation as with the keyboard shortcuts with the exception that I mergedmain in on my branch (which makes the diff look bigger than it is). You can either cherry-pick 69c83d33b1bb6c2349787983edf37aa55db1f0ae or you can merge my branch into yours. (doing the latter means you will get the merge with main "for free")

Cool glad you like it :) In terms of branches @franknoirot , I think you should have a go at the wrangling them, that's how you'll get comfortable doing it. Worst thing is you break both you the branches locally in which case you can delete them locally and pull them again from github for another attempt. If you're nervous about it you make a new branch off `sidebar-tray` as a dummy to test the sequence, and if it works you can do it for real on your original branch. This is pretty the same situation as with the keyboard shortcuts with the exception that I mergedmain in on my branch (which makes the [diff look bigger](https://github.com/Irev-Dev/cadhub/compare/sidebar-tray...kurt/sidebar-tray) than it is). You can either cherry-pick `69c83d33b1bb6c2349787983edf37aa55db1f0ae` or you can merge my branch into yours. (doing the latter means you will get the merge with main "for free")
franknoirot commented 2021-09-12 22:07:37 +02:00 (Migrated from github.com)
Copy Link

Alright hell yeah @Irev-Dev on it.

Alright hell yeah @Irev-Dev on it.
😆 1
franknoirot (Migrated from github.com) reviewed 2021-09-12 23:25:38 +02:00
franknoirot (Migrated from github.com) left a comment
Copy Link

@Irev-Dev I think it's set so that you can't approve / review because you opened the PR, but would you mind reviewing this for me when you're free? I think I got collapsing menus on ancestor menu click working, and I turned on the gear icon buttons in each of the panes, so fun to click around now.

@Irev-Dev I think it's set so that you can't approve / review because you opened the PR, but would you mind reviewing this for me when you're free? I think I got collapsing menus on ancestor menu click working, and I turned on the gear icon buttons in each of the panes, so fun to click around now.
app/web/src/components/IdeSideBar/sidebarConfig.tsx
@@ -0,0 +59,4 @@
rel="noreferrer"
>
Discord
</a>
franknoirot (Migrated from github.com) commented 2021-09-12 23:15:56 +02:00
Copy Link

Feels silly to put this here for temporary copy, but I really didn't want to rewrite the Discord link 3 times lol. Should I have done this differently?

Feels silly to put this here for temporary copy, but I really didn't want to rewrite the Discord link 3 times lol. Should I have done this differently?
app/web/src/components/IdeSideBar/sidebarConfig.tsx
@@ -0,0 +136,4 @@
key={'settings-tray-' + item.name}
open={state.sideTray.slice(-1)[0] === item.name}
onClick={(e) => {
e.preventDefault()
franknoirot (Migrated from github.com) commented 2021-09-12 23:20:09 +02:00
Copy Link

Unsure if e.preventDefault() on the <details> onClick event breaks any of the semantic/SR advantages of using it in the first place, but it doesn't appear to hurt keyboard navigation with Tab and Space which is cool.

Unsure if `e.preventDefault()` on the `<details>` `onClick` event breaks any of the semantic/SR advantages of using it in the first place, but it doesn't appear to hurt keyboard navigation with Tab and Space which is cool.
app/web/src/helpers/hooks/useIdeState.ts
franknoirot (Migrated from github.com) commented 2021-09-12 23:22:46 +02:00
Copy Link

This is how I implemented clicks on the main gear icon closing the whole menu. Does this work for you @Irev-Dev?

This is how I implemented clicks on the main gear icon closing the whole menu. Does this work for you @Irev-Dev?
Irev-Dev commented 2021-09-12 23:28:36 +02:00 (Migrated from github.com)
Copy Link

but would you mind reviewing this for me when you're free?

Sorry, side effect from me starting the PR. and yup should be able to take a look at this and the user profile PR after work today.

> but would you mind reviewing this for me when you're free? Sorry, side effect from me starting the PR. and yup should be able to take a look at this and the user profile PR after work today.
👍 1
Irev-Dev commented 2021-09-18 08:22:06 +02:00 (Migrated from github.com)
Copy Link

Looks good,
I quickly fixed a double icon that think is from the icon being included in the header when it already existed on some of the other pages.
image

Looks good, I quickly fixed a double icon that think is from the icon being included in the header when it already existed on some of the other pages. ![image](https://user-images.githubusercontent.com/29681384/133878548-5d259526-ab9b-4ee8-ab17-3686860f509e.png)
😆 1
Sign in to join this conversation.
Reviewers
No Reviewers
Irev-Dev
franknoirot
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
h3n3
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: h3n3/cadhub#507
Delete Branch "sidebar-tray"

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?