Skip to content
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

Vertical content cards #2470

Merged
merged 31 commits into from
Aug 20, 2024
Merged

Vertical content cards #2470

merged 31 commits into from
Aug 20, 2024

Conversation

GiseleN523
Copy link
Collaborator

@GiseleN523 GiseleN523 commented Aug 8, 2024

This PR adds as a list view of the content of a folder.

@GiseleN523 GiseleN523 requested a review from dqnykamp August 12, 2024 21:38
@dqnykamp
Copy link
Member

Something broke with tile view so that the tiles are no longer centered horizontally when there is just one or two of them.

Copy link
Member

@dqnykamp dqnykamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When there is no content, the Activities and the Assigned look quite bleak in list view with everything white and no visual distinction between the header and rest of the body. Maybe make it another color, like it is in card view, to give the page a little more texture.

Copy link
Member

@dqnykamp dqnykamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an interesting UI question for which I don't have a good answer. If one clicks an editable title and then clicks away to remove the focus from the title, should that click navigate to an activity if the click lands on an activity card? It does for both card view and list view.

For card view, I left it with the current behavior as I didn't know how to change it, but I'm not sure if it is the best choice. Especially in list view where one has the whole screen filled with activities, I'm worried there wouldn't be much "neutral" (non-link) screen space to click on to remove focus from the title without navigating to an activity.

Copy link
Member

@dqnykamp dqnykamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Activities, could you add a column with "Assignment status" or something like that shows if an assignment is open or closed (maybe blank if not assigned?). Also, in "Assigned" could you add an "Assignment status" or something like that header above that information?

Copy link
Member

@dqnykamp dqnykamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For responsive design, we want the screens to be able to go down to 344 pixels width (at least that is the smallest I get when selecting different phones in Chrome's web developer tools). For the list view, you'll want to compress the spacing for narrower screens. (See Chakra's Responsive Styles for info on how to do that.) For the narrower screens, we don't want any left-to-right scrolling. If you need to wrap text or stack items vertically, that's better than having horizontal scrolling.

…ntent, fixed assignment status column in activities, made it so that for card and list mode first click after blurring title textbox does nothing
@GiseleN523
Copy link
Collaborator Author

I think you're right about being able to click away from a textbox without triggering the click event on another row/card. I think I found a solution that works for the cards and table, and it feels a lot more natural that way, being able to click anywhere. I tested it and it seems to work as expected for both views (the click event is disabled one time and not any more after that), but let me know if you notice any weirdness with it.

@dqnykamp
Copy link
Member

I like it better this new way. I'm not sure what people expect. I guess we'll see how as people use it. The only worry is that the mouse cursor changes to a pointer when moving over a button, so I wonder if that will confuse people if it doesn't actually click the button on the first try. We'll have to test this out in usability studies at some point.

@dqnykamp
Copy link
Member

OK, I found some strange behavior. Click to edit a title on a folder/activity. Then select part of the title with your mouse, move your mouse pointer outside the title input so that it is still on the item and let go of the mouse pointer. It navigates to that item!

I don't know if we have to worry about this, but it definitely is strange!

…andles the case of dragging the text onto a link
@GiseleN523
Copy link
Collaborator Author

GiseleN523 commented Aug 16, 2024

Wow good catch! I had to get my brother to help replicate it, because I couldn't figure it out. I just pushed something that I think handles that case and hopefully doesn't break anything from before! I added a whole new state to address it, and I'm not sure if that's worth it, but I think it does fix it.

@dqnykamp
Copy link
Member

Great. Let me know if you want to add the responsive design components to this PR. Or, I can merge it in as is, and you can work on responsive design with another PR. I already deployed this to dev2.doenet.org so that other can check it out.

@GiseleN523
Copy link
Collaborator Author

Cool! It's up to you; I started it, but I'm not sure I'm going about it right and it could take me another couple of days. If you want to get this merged in soon, I don't mind making another PR.

<Th></Th>
{showPublicStatus ? <Th>Visibility</Th> : null}
<Show above="md">
{" "}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, having such white space " " (here and on the next line) isn't allowed inside a <tr>. It's triggering the warning (visible in the Javascript console):

Warning: validateDOMNesting(...):
Whitespace text nodes cannot appear as a child of <tr>. Make sure you don't have any extra whitespace between tags on each line of your source code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm I can't remembering adding that or figure out why it's needed. I think I'm just going to get rid of it, unless you know of any reason why we need it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't need it. It was probably something added by prettier at one point. I've noticed those appearing sometimes when formatting the document, but I'm not sure what causes them.


function saveUpdatedTitle(newTitle: string) {
if (newTitle !== activity.title && activity.id !== undefined) {
fetcher.submit(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't let the new title be a blank "" or you won't be able to change it back here, due to the <Editable> being too small. Instead, if newTitle==="", then just set the name to "Untitled".

Fix regression in changing content name from Doenet#2472
@GiseleN523
Copy link
Collaborator Author

In order to make things consistent, do you think it makes more sense to switch everything to be an Editable, as in ActivityTable, or an Input/Text combo like in ContentCard?

@dqnykamp
Copy link
Member

In order to make things consistent, do you think it makes more sense to switch everything to be an Editable, as in ActivityTable, or an Input/Text combo like in ContentCard?

I don't have a strong feeling, but you'd think there would be some benefits of Chakra's Editable. I'm not sure what. It's possible it's better designed for accessibility.

Copy link
Member

@dqnykamp dqnykamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving and merging this PR to make applying the next PRs easier. We can resolve remaining issues in another PR.

@dqnykamp dqnykamp merged commit 713b667 into Doenet:main Aug 20, 2024
7 checks passed
@GiseleN523 GiseleN523 deleted the verticalContentCards branch August 25, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants