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: Improve UI for Slur Crowdsource Feature #546

Merged
merged 6 commits into from
Apr 25, 2024

Conversation

Snehil-Shah
Copy link
Contributor

Resolves #409


name: feature
title: 'Improve UI for Slur Crowdsource Feature'
labels: enhancement


Made the slur card use the Card component instead of the Box component.
The design is kept the same for now with slight changes in padding. Some design changes can be iterated over. I personally like the current design, it's simple and straightforward

Steps to test the PR
I have swapped the component to use the new one in Slur.jsx, so you can use it as it is to test the changes

Expected behavior
The slur metadata in the slur list should look like the screenshots below.

Screenshots

card_dim

card_expand

@aatmanvaidya
Copy link
Collaborator

Hi, @Snehil-Shah thank you so much for the work

Had some feedback while reviewing the PR

  • In the SlurCard.js file, was the custom slur card component that we had written, and to structure the contents inside it, we had used the Table components from Grommet ( TableRow, TableCell, TableBody etc). This approach is very verbose as you can see, to get away from this, we wanted to use the Card Component from Grommet, hoping we don't have to add tables and all in it.
    • Currently, in your code you have added the entire table inside the CardBody, can you see if its possible to not add the table components and just use the card components to create this.

Please let me know if I can help clarify things even more!

@Snehil-Shah
Copy link
Contributor Author

@aatmanvaidya Ok, so the CardBody component is just a Box component with flex: true (acc to their docs). So it doesn't provide ways to layout content inside the body. We have to use a different component for that (like I used a Table previously).

So to get away from the verbosity, I used a DataTable instead to layout the content and the code is now much cleaner and abstracted and it looks great too:

image

image

We can of course change the things like which column is bolded etc.

Is it okay now?

@aatmanvaidya
Copy link
Collaborator

@Snehil-Shah this approach looks neat, give me some time, I will review it and get back to you as soon as possible

@Snehil-Shah
Copy link
Contributor Author

@aatmanvaidya Sure! Also, changed the font from bold to the original one, looks cleaner:

image

image

@aatmanvaidya
Copy link
Collaborator

Hi @Snehil-Shah, looks great

reviewed and tested out your PR locally

just two changes are required

  1. The footer where you have added the See More and See Less buttons, can you center them? as they originally were at the center?
  2. There is one more feature in Uli, where a user can select a word, right click on it and add the slur to Uli, once the user does this, the slur also gets added to the Crowdsourced Slur List. And since the user has only added the slur and not the metadata, the UI at this point looks like this

Screenshot from 2024-04-24 18-19-24

In your PR, in this case, the footer still exists, meaning when a user adds a slur by right clicking on it, the footer of (see more and see less buttons) is still there, which should not be there. Can you fix this?

To make sure the right-click on a word and add it to the slur list feature is working in your development setup, you will have make a small change.
If you are using Chrome, then in your mainfest.json file, make permissions section, add this http://localhost:3000/*, so your permissions should now looks like this

    "permissions": [
        "storage",
        "webRequest",
        "contextMenus",
        "https://ogbv-plugin.tattle.co.in/*",
        "http://localhost:3000/*"
    ],

Hope this helps, please feel free to reach out to me for more clarity!

@Snehil-Shah
Copy link
Contributor Author

Snehil-Shah commented Apr 24, 2024

image

@aatmanvaidya Done (ignore the pixelated screenshot, I am running it on a bad VM's browser)

I also think the warning message inside the card itself (notice the elevation and shadow around the warning) looks good, than just the warning. What do you think?

Also removed the old component. This PR should be now ready to merge...

@Snehil-Shah Snehil-Shah marked this pull request as ready for review April 24, 2024 15:56
@aatmanvaidya
Copy link
Collaborator

Hi @Snehil-Shah , yes the warning message inside the card looks good!

Thank you so much for your work on this, this looks good, I have tested it out locally, its ready to merge!

@aatmanvaidya aatmanvaidya merged commit 8662ca5 into tattle-made:main Apr 25, 2024
3 checks passed
@dennyabrain
Copy link
Contributor

hey @Snehil-Shah I'm hijacking this comment section for a separate discussion. I noticed your project https://github.com/Snehil-Shah/Multimodal-Image-Search-Engine and wanted to point you towards another project of ours called Feluda that has two open projects in the DMP 2024 program that you might interest you. They are pretty much doing what you did in yoru repo but for audio and video. Here are the links - tattle-made/feluda#82, tattle-made/feluda#81

see if they are of interest.
Thank you again for the work on this PR

@Snehil-Shah
Copy link
Contributor Author

@dennyabrain Hey, thanks for checking out the project! Feluda seems interesting, will do some R&D around it asap..👍

@Snehil-Shah Snehil-Shah deleted the slur-card-ui branch April 26, 2024 12:23
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.

Improve UI for Slur Crowdsource Feature
3 participants