Resources Management UX Merge #5
Conversation
There was a problem hiding this comment.
I left some comments on this PR, some of which might be addressed in a later ticket (they are marked with a 💡). Some are small changes, some are things to consider, some are just suggestions. Feel free to push back on any of it. Also, comments might be out of order in the main feed-- I was jumping around in the files so it might be a bit of back and forth.
Lastly, I noticed a small bug having to do with drag and drop, which is a larger discussion since we may consider implementing a library later to make it easier. I've attached a video to show the issue.
Screen.Recording.2024-09-17.at.4.32.16.PM.mov
It seems like you can drag links into the left hand side and it creates a div for it there. Not sure what the best fix is for this.
| <StaticContentCard | ||
| title="Kolibri" | ||
| description="Kolibri provides an extensive library of educational content suitable for all learning levels." | ||
| imgSrc={KolibriImg} | ||
| altText="Kolibri logo" | ||
| linkUrl="https://kolibri.v2.unlockedlabs.xyz/oidcauthenticate/" | ||
| linkText="Explore Kolibri's Content" | ||
| /> |
There was a problem hiding this comment.
instead of passing this info in like this, the idea was that we would call the backend to render all of these that are stored in the backend - i think the right call is a GET to /api/open-content but I would double check on that. Hard coding was just a temporary solution for the demo, but now that we've updated this might as well do it completely.
There was a problem hiding this comment.
Also, rather than passing in each individual component, it might just be easier to pass in the whole open content provider (as in pass in the whole object that is returned, such as Kolibri, which contains the following fields since it is of type OpenContentProvider:
type OpenContentProvider struct {
DatabaseFields
Url string `gorm:"size:255;not null;unique" json:"url"`
ProviderPlatformID uint `json:"provider_platform_id"`
Thumbnail string `json:"thumbnail_url"`
CurrentlyEnabled bool `json:"currently_enabled"`
Description string `json:"description"`
ProviderPlatform *ProviderPlatform `gorm:"foreignKey:ProviderPlatformID;constraint:OnDelete SET NULL" json:"-"`
}
and then you would access each of these in the StaticContentCard.tsx. See comment on this file for more reference.
There was a problem hiding this comment.
I agree with you but would ask if we can consider it out of scope for this task. I don't see Content Provider types defined in the frontend or backend seed data for Open Content Providers. Do you?
There was a problem hiding this comment.
Right, I see what you are saying. But, I will say that I don't think that updating this StaticContent card is in the scope of this task either. I suggested this change because it feels like if we are going to create a new component, we should probably update and fix it completely. I'm okay with creating a different task for this, but in the future maybe a new task should be made to handle the entire issue? I might be missing something regarding why this was updated in this ticket, though, so please let me know if so!
| interface StaticContentCardProps { | ||
| title: string; | ||
| description: string; | ||
| imgSrc: string; | ||
| altText: string; | ||
| linkUrl: string; | ||
| linkText: string; | ||
| } | ||
|
|
||
| export default function StaticContentCard({ | ||
| title, | ||
| description, | ||
| imgSrc, | ||
| altText, | ||
| linkUrl, | ||
| linkText | ||
| }: StaticContentCardProps) { |
There was a problem hiding this comment.
So by passing in an object of type OpenContentProvider, we could eliminate all of these lines of code. It would be reduced down to just export default function StaticContentCard({provider}: {provider: OpenContentProvider}) { .. rest of code ....}. Also, I would say that StaticContentCard might not be the best thing to name this component. Because this card will only be used for Open Content providers, I would name it as such. That's just how I would approach it though- not sure if there was some other motive behind the naming of this component.
There was a problem hiding this comment.
When writing this I didn't realize that there was some dynamic nature to our content providers. I'd say see my previous comment again and maybe a future task can be to remove static providers and replace it with dynamic calls to the API to get Open Content Providers?
| return ( | ||
| <div className="card card-compact bg-base-teal overflow-hidden"> | ||
| <img | ||
| src={imgSrc} |
There was a problem hiding this comment.
so then down here, you would access the info by doing provider.imgSrc. Again, this is how I would approach it but feel free to push back!
| <div className="card-body gap-2"> | ||
| <div className="flex justify-between"> | ||
| <h3 className="card-title text-sm">{collection.name}</h3> |
There was a problem hiding this comment.
💡 card-body and card-title are built into daisyUI and don't fit our styling. the styling tags we can use are listed in app.css. Its a bit confusing because we use daisyUI in some places, but our own styled components in other places, but I am trying to shift away from using daisyUI and more towards building our own components/styling so we have complete control over everything we create. Daisy is super complicated to alter their base styles, so when in doubt look at the app.css file or do it inline (if it is not a universal style that will be used other places). For this, the <h3> should already have the necessary styling, so you can remove the whole className="card-title text-sm". the div that hascard-body can be removed completely, and instead we can just add py-4 px-6 to the div that has className="card" (it might be worth looking into the padding for card components in general and making this a universal padding used. but, we need to make sure it wont impact the other layouts that use card and evaluate from there).
jtucholski
force-pushed
the
modal_merge
branch
from
95b7613 to
6c70dcf
Compare
|
@calisio With respect to the bug you observed I think we need to restrict what types of elements can be dropped onto a drag/drop container. We could look into fixing that here or we might want to make that part of the ticket that goes with replacing the functionality with a drag and drop library. What do you think? |
jtucholski
force-pushed
the
modal_merge
branch
from
6c70dcf to
97aa955
Compare
|
Honestly, ya I think that we should just leave it for now and fix it when we add a drag drop library. If bug isn't breaking the functionality maybe we just leave it for later? |
jtucholski
force-pushed
the
modal_merge
branch
2 times, most recently
from
3774e4e to
23437a0
Compare
Description of the change
This PR largely focuses on changes to the Resources Management page. There are a lot of areas where the code can be refactored after which would extend into creation of new components & types.
Screenshot(s)