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

feat: Adding more informations on My Shares page (table and modal) #174

Merged
merged 17 commits into from Jun 26, 2023

Conversation

pierrbt
Copy link
Contributor

@pierrbt pierrbt commented Jun 25, 2023

The description of the share is now displayed out on the table of My Shares page and by clicking the new button Information, a Modal pops out, on which you can see multiples informations about the share, as shown below.

This Pull Request is related to the Issue #164 . Do not hesitate if you have any other ideas to add on the page, or if you want to modify these changes.

Images :

image
image

Copy link
Owner

@stonith404 stonith404 left a comment

Choose a reason for hiding this comment

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

Awesome work, thank you!
A few things that I have noticed:

  • Is the copy link button in the table still necessary? Because you can copy the link in the model. If you want to remove it, I would change the color of the info icon to the primary color (purple)
  • Don't you think we could completely remove the author property in the model? Because its value is hard coded
  • If the description is too long, the table doesn't look nice in my opinion, maybe you could limit the description length in the table but show the whole description in the modal:
image

Btw. I really like the idea of the progress bar for the share size :)

@pierrbt
Copy link
Contributor Author

pierrbt commented Jun 25, 2023

I'll see that asap, thank you for the review

pierrbt and others added 5 commits June 25, 2023 21:06
Co-authored-by: Elias Schneider <login@eliasschneider.com>
Co-authored-by: Elias Schneider <login@eliasschneider.com>
Co-authored-by: Elias Schneider <login@eliasschneider.com>
Co-authored-by: Elias Schneider <login@eliasschneider.com>
Co-authored-by: Elias Schneider <login@eliasschneider.com>
@pierrbt
Copy link
Contributor Author

pierrbt commented Jun 25, 2023

Awesome work, thank you!
I'm happy to help on this project !

Is the copy link button in the table still necessary? Because you can copy the link in the model.

Yes for sure, but I think the copy link button remains important to make the copy faster, and not disturb people that copied the link like that, I don't really know, but it's not that disturbing

Don't you think we could completely remove the author property in the model? Because its value is hard coded

Yes, I can remove it, indeed the user is in My Shares, so he knows that it's his share.

If the description is too long, the table doesn't look nice in my opinion, maybe you could limit the description length in the table but show the whole description in the modal

I'll work on this, I can limit the height, but when I try working on the width, I can only use px, I've already had this issue using tables before, but I don't know how to solve it, I put a 300px max-width, so it's good on PC but on smartphones, it's terrible.

image

Btw. I really like the idea of the progress bar for the share size :)

Yes I found that it would fill a bit the interface and the result is good.

@pierrbt
Copy link
Contributor Author

pierrbt commented Jun 25, 2023

I'll work on this, I can limit the height, but when I try working on the width, I can only use px, I've already had this issue using tables before, but I don't know how to solve it, I put a 300px max-width, so it's good on PC but on smartphones, it's terrible.

I've found a way to bypass this error :

display: table;
table-layout: fixed;

But the problem is, by setting the layout to fixed, the table cannot slide as before.
Maybe we could just hide the description on smaller screens, because anyway we'll not be able to see it

@stonith404
Copy link
Owner

Yes for sure, but I think the copy link button remains important to make the copy faster, and not disturb people that copied the link like that, I don't really know, but it's not that disturbing

Yeah good point.

@stonith404
Copy link
Owner

Maybe we could just hide the description on smaller screens, because anyway we'll not be able to see it

Yes I think this would be the best solution as the description can be seen anyways if you open the modal on mobile.

@pierrbt
Copy link
Contributor Author

pierrbt commented Jun 25, 2023

@stonith404

Yes I think this would be the best solution as the description can be seen anyways if you open the modal on mobile.

Do you know how we can do this directly in React ?
Else we can just make a media query

@stonith404
Copy link
Owner

@pierrbt Mantine does support media queries https://mantine.dev/core/media-query/

Something like this:

      <MediaQuery largerThan="lg">
           {/* the description in here */}
      </MediaQuery>

@pierrbt
Copy link
Contributor Author

pierrbt commented Jun 25, 2023

@stonith404
Just added a MediaQuery

@stonith404
Copy link
Owner

@pierrbt LGTM, thanks again!

@stonith404 stonith404 merged commit 1466240 into stonith404:main Jun 26, 2023
1 check passed
@pierrbt
Copy link
Contributor Author

pierrbt commented Jun 26, 2023

@stonith404
No problems, I had time to help.
PS : this week I'll not be able to code, I'm not at home

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.

None yet

2 participants