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

Profile page redesign #510

Merged
franknoirot merged 11 commits from profile-page into main 2021-09-18 07:16:33 +02:00
Conversation 19 Commits 11 Files Changed 21 +711 -417
franknoirot commented 2021-09-12 19:57:58 +02:00 (Migrated from github.com)
Copy Link

Major Changes

  1. User Profile: Owner view
    Screen Shot 2021-09-12 at 1 04 22 PM
  2. User Profile: Non-Owner view
    Screen Shot 2021-09-12 at 1 47 52 PM
  3. User Profile: Field edit modes
    Screen Shot 2021-09-12 at 1 49 38 PM
  4. New look for Account Recovery and Pwd Change pages (they look basically the same so only putting on screenshot)
    Screen Shot 2021-09-12 at 1 23 17 PM

Minor Changes

  1. Tweaked signed-in popover:
    Screen Shot 2021-09-12 at 1 03 33 PM
  2. Broke out an ImageFallback component from ImageUploader without the upload functionality, because I couldn't have a button within a button for Popover rework to migrate to HeadlessUI.
  3. Added hover state to ProjectCard
  4. Added ability for KeyValue's key to appear below or above its value (for use with Name and UserName in profile)
## Major Changes 1. User Profile: Owner view ![Screen Shot 2021-09-12 at 1 04 22 PM](https://user-images.githubusercontent.com/23481541/132997622-02cfd164-0ea1-4a4e-bf16-ac246a97b3d2.jpg) 2. User Profile: Non-Owner view ![Screen Shot 2021-09-12 at 1 47 52 PM](https://user-images.githubusercontent.com/23481541/132997642-a126d547-45c6-46e9-96d3-0fa5e7bd921f.jpg) 3. User Profile: Field edit modes ![Screen Shot 2021-09-12 at 1 49 38 PM](https://user-images.githubusercontent.com/23481541/132997663-2fd57206-39bc-450c-9640-8cdb5ccdfeed.jpg) 4. New look for Account Recovery and Pwd Change pages (they look basically the same so only putting on screenshot) ![Screen Shot 2021-09-12 at 1 23 17 PM](https://user-images.githubusercontent.com/23481541/132996919-ba9f9ab1-70c1-4b73-947e-d0911eb0602f.jpg) ## Minor Changes 1. Tweaked signed-in popover: ![Screen Shot 2021-09-12 at 1 03 33 PM](https://user-images.githubusercontent.com/23481541/132996776-c5763b77-e680-4aa4-be4a-03459bfea940.jpg) 2. Broke out an `ImageFallback` component from `ImageUploader` without the upload functionality, because I couldn't have a button within a button for `Popover` rework to migrate to HeadlessUI. 3. Added hover state to `ProjectCard` 4. Added ability for `KeyValue`'s key to appear below or above its value (for use with Name and UserName in profile)
🎉 1
franknoirot commented 2021-09-12 20:02:12 +02:00 (Migrated from github.com)
Copy Link

@Irev-Dev let me know if my habit I've picked up of breaking things into a config file makes things more confusing, that's what I think the weakest point of this is.

I do think that what I've done does an alright job of showing just what data is available to a given profile field within UserProfile.tsx, so at a glance you can see if a field is editable if it takes the hasPermissionToEdit prop.

@Irev-Dev let me know if my habit I've picked up of breaking things into a config file makes things more confusing, that's what I think the weakest point of this is. I do think that what I've done does an alright job of showing just what data is available to a given profile field within `UserProfile.tsx`, so at a glance you can see if a field is editable if it takes the `hasPermissionToEdit` prop.
franknoirot (Migrated from github.com) reviewed 2021-09-12 20:23:59 +02:00
app/web/src/components/IdeHeader/IdeHeader.tsx
@@ -187,0 +142,4 @@
<Svg name="camera" className="w-6 h-6 text-ch-blue-400" />
</TopButton>
)}
/>
franknoirot (Migrated from github.com) commented 2021-09-12 20:12:23 +02:00
Copy Link

Not sure this is the right move but I made it so a component could override the default buttons by bringing their own as children to it. It let me remove them by providing a dummy element without effecting any other pages.

Not sure this is the right move but I made it so a component could override the default buttons by bringing their own as children to it. It let me remove them by providing a dummy element without effecting any other pages.
app/web/src/components/IdeHeader/IdeHeader.tsx
franknoirot (Migrated from github.com) commented 2021-09-12 20:13:18 +02:00
Copy Link

Nothing new in here, just moved into its own component for the override ability above.

Nothing new in here, just moved into its own component for the override ability above.
app/web/src/components/ImageUploader/ImageUploader.js
@@ -11,6 +11,26 @@ import Svg from 'src/components/Svg'
const CLOUDINARY_UPLOAD_PRESET = 'CadHub_project_images'
const CLOUDINARY_UPLOAD_URL = 'https://api.cloudinary.com/v1_1/irevdev/upload'
export function ImageFallback({
franknoirot (Migrated from github.com) commented 2021-09-12 20:15:15 +02:00
Copy Link

Having a component that got that fallback user image without the image uploading functionality made me able to migrate the signed-in popover to HeadlessUI from MaterialUI, because I couldn't have a button within a button. I figured it's okay because the use case for the top nav (and ProjectCard) don't ever need the upload functionality.

Having a component that got that fallback user image without the image uploading functionality made me able to migrate the signed-in popover to HeadlessUI from MaterialUI, because I couldn't have a button within a button. I figured it's okay because the use case for the top nav (and `ProjectCard`) don't ever need the upload functionality.
app/web/src/components/KeyValue/KeyValue.tsx
@@ -0,0 +29,4 @@
interface KeyValueType {
keyName: string
children: React.ReactNode
bottom?: boolean
franknoirot (Migrated from github.com) commented 2021-09-12 20:16:28 +02:00
Copy Link

Allows the key to appear below (only visually) using CSS order property,

Allows the key to appear below (only visually) using CSS `order` property,
app/web/src/components/KeyValue/KeyValue.tsx
@@ -0,0 +34,4 @@
edit?: EditToggleType
}
const KeyValue = ({
franknoirot (Migrated from github.com) commented 2021-09-12 20:17:12 +02:00
Copy Link

Broke out into its own component to be used across both PartProfile and UserProfile

Broke out into its own component to be used across both `PartProfile` and `UserProfile`
app/web/src/components/UserProfile/UserProfile.tsx
franknoirot (Migrated from github.com) commented 2021-09-12 20:18:51 +02:00
Copy Link

Initializes values from user state into the config object. Is this a code smell?

Initializes values from `user` state into the config object. Is this a code smell?
app/web/src/components/UserProfile/userProfileConfig.tsx
@@ -0,0 +19,4 @@
loading: boolean
error: boolean
onSave: Function
projects: {}[]
franknoirot (Migrated from github.com) commented 2021-09-12 20:20:29 +02:00
Copy Link

Is there a ProjectType defined anywhere? I couldn't find one.

Is there a `ProjectType` defined anywhere? I couldn't find one.
app/web/src/layouts/MainLayout/MainLayout.js
@@ -147,3 +171,3 @@
</button>
</Popover>
</li>
) : (
franknoirot (Migrated from github.com) commented 2021-09-12 20:23:08 +02:00
Copy Link

Migration from MaterialUI to HeadlessUI Popover.

Migration from MaterialUI to HeadlessUI Popover.
Irev-Dev (Migrated from github.com) reviewed 2021-09-14 12:46:08 +02:00
app/web/src/components/ImageUploader/ImageUploader.js
@@ -11,6 +11,26 @@ import Svg from 'src/components/Svg'
const CLOUDINARY_UPLOAD_PRESET = 'CadHub_project_images'
const CLOUDINARY_UPLOAD_URL = 'https://api.cloudinary.com/v1_1/irevdev/upload'
export function ImageFallback({
Irev-Dev (Migrated from github.com) commented 2021-09-14 12:46:08 +02:00
Copy Link

That button within a button error in the console has been annoying me for ages, but obviously I hadn't done anything about, so thanks!

That button within a button error in the console has been annoying me for ages, but obviously I hadn't done anything about, so thanks!
Irev-Dev (Migrated from github.com) reviewed 2021-09-14 13:04:13 +02:00
app/web/src/components/IdeHeader/IdeHeader.tsx
@@ -187,0 +142,4 @@
<Svg name="camera" className="w-6 h-6 text-ch-blue-400" />
</TopButton>
)}
/>
Irev-Dev (Migrated from github.com) commented 2021-09-14 13:04:13 +02:00
Copy Link

Hmm I was trying to figure out what this even needed to be modified for the profile page but because this essentially the top nav bar.

I think this approach is good.

Perhaps later we'll look to remove MainLayout.js and replace it with the component everywhere. i.e. it can be the have the search bar for the projects explorer etc. If we do that than displaying children ONLY is probably better, i.e. it will need DefaultTopButtons to be passed in, in this case.

This is good though.

Hmm I was trying to figure out what this even needed to be modified for the profile page but because this essentially the top nav bar. I think this approach is good. Perhaps later we'll look to remove `MainLayout.js` and replace it with the component everywhere. i.e. it can be the have the search bar for the projects explorer etc. If we do that than displaying children ONLY is probably better, i.e. it will need `DefaultTopButtons` to be passed in, in this case. This is good though.
Irev-Dev (Migrated from github.com) reviewed 2021-09-14 13:06:34 +02:00
app/web/src/components/ImageUploader/ImageUploader.js
@@ -13,1 +13,4 @@
export function ImageFallback({
width = 100,
imageId = 'CadHub/eia1kwru54g2kf02s2xx',
Irev-Dev (Migrated from github.com) commented 2021-09-14 13:06:34 +02:00
Copy Link

not relevant to this PR, but I do think there must be a better way of doing a fallback/empty-state than this image.

not relevant to this PR, but I do think there must be a better way of doing a fallback/empty-state than this image.
Irev-Dev (Migrated from github.com) reviewed 2021-09-14 13:16:39 +02:00
app/web/src/components/KeyValue/KeyValue.tsx
@@ -0,0 +47,4 @@
<div
className={
'text-ch-blue-400 font-fira-code flex items-center leading-4 text-sm whitespace-nowrap ' +
(bottom ? 'order-2' : '')
Irev-Dev (Migrated from github.com) commented 2021-09-14 13:16:39 +02:00
Copy Link

I'm not requesting a change this is just an observation. I noticed in the headlessui docs they follow a pattern like this for condition classes

className={`${active && 'bg-blue-500'}`}

At first I didn't like it because the result will often be className="false" so even className="false false" if you have more of them, but I guess there's no harm in having classes that don't do anything in there

I'm not requesting a change this is just an observation. I noticed in the [headlessui docs](https://headlessui.dev/react/menu#basic-example) they follow a pattern like this for condition classes ``` className={`${active && 'bg-blue-500'}`} ``` At first I didn't like it because the result will often be `className="false"` so even `className="false false"` if you have more of them, but I guess there's no harm in having classes that don't do anything in there
Irev-Dev (Migrated from github.com) reviewed 2021-09-14 13:27:37 +02:00
app/web/src/components/KeyValue/KeyValue.tsx
@@ -0,0 +34,4 @@
edit?: EditToggleType
}
const KeyValue = ({
Irev-Dev (Migrated from github.com) commented 2021-09-14 13:27:37 +02:00
Copy Link

I know you like this component but I kinda hate it, well hate is a sting word but the amount of input params is off putting for such a simple component. If we pulling it out to be reused I might be good to make a few changes.

  • hide should be removed entirely, why did I added it? like should we add a hid param to every component that's conditionally rendered 😩 . Cases where it was being used should just be replaced with `{!shouldHide && <KeyValue ... />}
  • isEditable is a terrible name, The variable is for when the component is in a edit state, but i reads like whether the component is allowed to be edited or something. perhaps 'inEditMode`
  • I liked your variable name hasPermissionToEdit could we rename canEdit to that as well? much clearer.

How does that sound, would you mind?

I know you like this component but I kinda hate it, well hate is a sting word but the amount of input params is off putting for such a simple component. If we pulling it out to be reused I might be good to make a few changes. - hide should be removed entirely, why did I added it? like should we add a hid param to every component that's conditionally rendered 😩 . Cases where it was being used should just be replaced with `{!shouldHide && <KeyValue ... />} - `isEditable` is a terrible name, The variable is for when the component is in a edit state, but i reads like whether the component is allowed to be edited or something. perhaps 'inEditMode` - I liked your variable name `hasPermissionToEdit` could we rename `canEdit` to that as well? much clearer. How does that sound, would you mind?
Irev-Dev (Migrated from github.com) reviewed 2021-09-14 13:30:02 +02:00
app/web/src/components/ProjectCard/ProjectCard.tsx
Irev-Dev (Migrated from github.com) commented 2021-09-14 13:30:02 +02:00
Copy Link

Whats the purpose of these comments?

Whats the purpose of these comments?
Irev-Dev commented 2021-09-14 13:36:30 +02:00 (Migrated from github.com)
Copy Link

Well I at least made a start this time.

I'm happy with everything I've looked at so far, but I skipped UserProfile.tsx. Seems to be the most intense part of the PR and it's past my bedtime. I'll get back to it soon.

Well I at least made a start this time. I'm happy with everything I've looked at so far, but I skipped `UserProfile.tsx`. Seems to be the most intense part of the PR and it's past my bedtime. I'll get back to it soon.
👍 1
franknoirot (Migrated from github.com) reviewed 2021-09-14 14:12:47 +02:00
app/web/src/components/ImageUploader/ImageUploader.js
@@ -13,1 +13,4 @@
export function ImageFallback({
width = 100,
imageId = 'CadHub/eia1kwru54g2kf02s2xx',
franknoirot (Migrated from github.com) commented 2021-09-14 14:12:47 +02:00
Copy Link

I hear that. I'll add to my list, maybe Cloudinary has a documented best.

I hear that. I'll add to my list, maybe Cloudinary has a documented best.
franknoirot (Migrated from github.com) reviewed 2021-09-14 14:18:07 +02:00
app/web/src/components/KeyValue/KeyValue.tsx
@@ -0,0 +47,4 @@
<div
className={
'text-ch-blue-400 font-fira-code flex items-center leading-4 text-sm whitespace-nowrap ' +
(bottom ? 'order-2' : '')
franknoirot (Migrated from github.com) commented 2021-09-14 14:18:07 +02:00
Copy Link

Oh interesting, yeah that's what I was avoiding here, but it would be shorter code their way. I guess the falses would have a negligible impact on things really.

Oh interesting, yeah that's what I was avoiding here, but it would be shorter code their way. I guess the `false`s would have a negligible impact on things really.
franknoirot (Migrated from github.com) reviewed 2021-09-14 14:54:33 +02:00
app/web/src/components/ProjectCard/ProjectCard.tsx
franknoirot (Migrated from github.com) commented 2021-09-14 14:54:33 +02:00
Copy Link

Sorry I was debugging an issue, I'll get these removed.

Sorry I was debugging an issue, I'll get these removed.
Irev-Dev (Migrated from github.com) requested changes 2021-09-14 22:17:06 +02:00
Irev-Dev (Migrated from github.com) left a comment
Copy Link

Couple things I noticed, I had trouble with editing the bio and having the change remain after a refresh, not sure if this a bug in existing code or not:

https://user-images.githubusercontent.com/29681384/133326590-f94cac32-4498-45a2-ad8c-13f7d67c651d.mov

And I noticed that the toast pops in quickly when navigation to one of the projects.

https://user-images.githubusercontent.com/29681384/133327626-a8e53dbf-89e1-4035-b8e5-38fb1185a282.mov

I think this is because <Toaster /> isn't on this page anywhere but is trying to be used after editing fields so it quickly shows when you go to a page that does have the component.

Other than that looks good, UserProfile seems a little complex but that might just be because I'm not used to this style.

@Irev-Dev let me know if my habit I've picked up of breaking things into a config file makes things more confusing

I think the good part of putting everything in a config is that your thinking about the UI in terms of data, though one thing to watch out for is not to get too carried away with abstractions, It's not directly related but I found the perspective in this blog post to be pretty handy https://overreacted.io/goodbye-clean-code/

Couple things I noticed, I had trouble with editing the bio and having the change remain after a refresh, not sure if this a bug in existing code or not: https://user-images.githubusercontent.com/29681384/133326590-f94cac32-4498-45a2-ad8c-13f7d67c651d.mov And I noticed that the toast pops in quickly when navigation to one of the projects. https://user-images.githubusercontent.com/29681384/133327626-a8e53dbf-89e1-4035-b8e5-38fb1185a282.mov I think this is because `<Toaster />` isn't on this page anywhere but is trying to be used after editing fields so it quickly shows when you go to a page that does have the component. Other than that looks good, UserProfile seems a little complex but that might just be because I'm not used to this style. > @Irev-Dev let me know if my habit I've picked up of breaking things into a config file makes things more confusing I think the good part of putting everything in a config is that your thinking about the UI in terms of data, though one thing to watch out for is not to get too carried away with abstractions, It's not directly related but I found the perspective in this blog post to be pretty handy https://overreacted.io/goodbye-clean-code/
franknoirot (Migrated from github.com) reviewed 2021-09-15 03:10:10 +02:00
app/web/src/components/KeyValue/KeyValue.tsx
@@ -0,0 +34,4 @@
edit?: EditToggleType
}
const KeyValue = ({
franknoirot (Migrated from github.com) commented 2021-09-15 03:10:09 +02:00
Copy Link

Yeah totally, love these ideas. I hear you it about the input params too.

Yeah totally, love these ideas. I hear you it about the input params too.
franknoirot (Migrated from github.com) reviewed 2021-09-15 03:12:15 +02:00
app/web/src/components/IdeHeader/IdeHeader.tsx
@@ -187,0 +142,4 @@
<Svg name="camera" className="w-6 h-6 text-ch-blue-400" />
</TopButton>
)}
/>
franknoirot (Migrated from github.com) commented 2021-09-15 03:12:15 +02:00
Copy Link

Yes I like that idea about MainLayout, it's feeling about ready to be taken out after a little refactor of this header to cover more use cases.

Yes I like that idea about `MainLayout`, it's feeling about ready to be taken out after a little refactor of this header to cover more use cases.
franknoirot (Migrated from github.com) reviewed 2021-09-15 08:06:03 +02:00
franknoirot (Migrated from github.com) left a comment
Copy Link

@Irev-Dev Alright I feel a lot better about this now. Here's what I got done:

  • Fix up KeyValue component
  • Remove comments from ProjectCard
  • See what’s up with the Markdown editor not saving.
  • Add Toaster
  • Find a less complex model for the User Profile
@Irev-Dev Alright I feel a lot better about this now. Here's what I got done: - Fix up KeyValue component - Remove comments from ProjectCard - See what’s up with the Markdown editor not saving. - Add Toaster - Find a less complex model for the User Profile
app/web/src/components/KeyValue/KeyValue.tsx
@@ -0,0 +34,4 @@
edit?: EditToggleType
}
const KeyValue = ({
franknoirot (Migrated from github.com) commented 2021-09-15 07:57:54 +02:00
Copy Link

I think I found something better with it! Take a look at it.

I think I found something better with it! Take a look at it.
app/web/src/components/UserProfile/UserProfile.tsx
@@ -0,0 +69,4 @@
</IdeHeader>
</div>
<div className="relative flex-grow h-full">
<div className="grid md:grid-cols-profile-layout grid-flow-row-dense absolute inset-0">
franknoirot (Migrated from github.com) commented 2021-09-15 08:00:08 +02:00
Copy Link

Check this out @Irev-Dev, I think I found something way better here. Using components instead of that big old config object.

Check this out @Irev-Dev, I think I found something way better here. Using components instead of that big old config object.
franknoirot commented 2021-09-15 08:07:09 +02:00 (Migrated from github.com)
Copy Link

Also such a good catch on the <Toaster/> issue

Also such a good catch on the `<Toaster/>` issue
franknoirot (Migrated from github.com) reviewed 2021-09-15 16:21:57 +02:00
app/web/src/components/KeyValue/KeyValue.tsx
@@ -0,0 +29,4 @@
interface KeyValueType {
keyName: string
children: React.ReactNode
bottom?: boolean
franknoirot (Migrated from github.com) commented 2021-09-15 16:19:33 +02:00
Copy Link

These comments aren't showing up as out of date which is worrying me @Irev-Dev , so weird. This component was overhauled, see this commit

These comments aren't showing up as out of date which is worrying me @Irev-Dev , so weird. This component was overhauled, see [this commit](https://github.com/Irev-Dev/cadhub/commit/7b2be01430454fb599ec9448de427c19db55e7c5#diff-bbd6cdd539b9e9bc9272aa05cb7894082d87d6772f64ed849ba0385ba960e5a4R29-R35)
Irev-Dev (Migrated from github.com) approved these changes 2021-09-18 07:16:23 +02:00
Irev-Dev (Migrated from github.com) left a comment
Copy Link

Looks really god.

Sorry it took so long for me to get back to this.

Looks really god. Sorry it took so long for me to get back to this.
Sign in to join this conversation.
Reviewers
No Reviewers
franknoirot
Irev-Dev
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#510
Delete Branch "profile-page"

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?