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

#895 - Improve table used on the /settings/api page #976

Merged
merged 16 commits into from Jul 22, 2022
Merged

#895 - Improve table used on the /settings/api page #976

merged 16 commits into from Jul 22, 2022

Conversation

rupin90
Copy link
Contributor

@rupin90 rupin90 commented Jul 15, 2022

Here is the behavior of the Table on different screen sizes. Finally was able to lay out the CSS. Somehow it wasn't as straight forward as I thought.
Leveraged NPM package - https://github.com/coston/react-super-responsive-table

Remaining Items:-

  • - border and background CSS for the table rows
  • - Any feedback
localhost_3000_settings_api.1.mp4

@vercel
Copy link

vercel bot commented Jul 15, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
omnivore-demo ✅ Ready (Inspect) Visit Preview Jul 22, 2022 at 2:31PM (UTC)
omnivore-prod ✅ Ready (Inspect) Visit Preview Jul 22, 2022 at 2:31PM (UTC)

@jacksonh
Copy link
Contributor

I think this is just starting out, right? I notice that when scaling the table down the headers don't collapse correctly:

https://www.loom.com/share/e796b6f5aae444ec9aae7b736e4d5d76

@rupin90
Copy link
Contributor Author

rupin90 commented Jul 18, 2022

Sorry, this is still in progress. I was just testing this out if this is a good one to work on.

@jacksonh - When you said that headers don't collapse, you mean - we don't want to show headers at all on small screens?
I thought we want to show headers 😓 .

@rupin90 rupin90 self-assigned this Jul 18, 2022
@rupin90 rupin90 added the Work in Progress Work in Progress label Jul 18, 2022
@jacksonh
Copy link
Contributor

@rupin90 oh sorry i mean they dont move with the table, we do want to show them

@jacksonh
Copy link
Contributor

Hey @rupin90 I noticed two things:

  1. It looks like we should import TableR instead of table. Type error: '"../../components/elements/Table"' has no exported member named 'Table'. Did you mean 'TableR'?

  2. For some reason the z-index or something is higher with the table than the modals. When I open the modal all the headers of the table appear on top of the modal.

Screen Shot 2022-07-19 at 15 31 56

@rupin90
Copy link
Contributor Author

rupin90 commented Jul 20, 2022

Hey @rupin90 I noticed two things:

  1. It looks like we should import TableR instead of table. Type error: '"../../components/elements/Table"' has no exported member named 'Table'. Did you mean 'TableR'?
  2. For some reason the z-index or something is higher with the table than the modals. When I open the modal all the headers of the table appear on top of the modal.
Screen Shot 2022-07-19 at 15 31 56

Thank you, wil look into it tonight 🙏

@rupin90
Copy link
Contributor Author

rupin90 commented Jul 20, 2022

I have updated the name to TableNew, I can update to any other name if you have any suggestions. This is simply to avoid conflict with the library. It is working fine.

Z-index issue is fixed now. Modal didn't have any z-index so I just gave it to Modal since that would be always be the top view if open.

@rupin90
Copy link
Contributor Author

rupin90 commented Jul 20, 2022

Also I have tried different ways to customize the layout of the table - it has been very tricky so I have asked the community on how to customize the look and feel for the table - coston/react-super-responsive-table#382

The css prop/attribute does not work on these elements. :(.

I also tried the typical way adding a className but the output is like
<div class=[object object]>........ </div>

Not sure if this was faced before in our codebase.

CC: @jacksonh @satindar

@rupin90
Copy link
Contributor Author

rupin90 commented Jul 21, 2022

Hey @rupin90 this is really good.

I just pushed a commit to show how to style these imported elements, i also renamed the import just so we could use Table elsewhere.

with stitches we use this stytled function to make a component stylable. It also works on strings like styled('br', { .. a bunch of css. You can add styling in that styled call, or any component that you've made styled, then you can use the css attribute.

Aah nice, Thank you 🙏🏽 . Will do it just now.

@@ -102,7 +102,7 @@ export function TableNew(props: TableProps): JSX.Element {
},
}}
>
<Table>
<ResponsiveTable>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👏🏽 Cool!

@@ -106,7 +106,7 @@ export function Table(props: TableProps): JSX.Element {
},
}}
>
<StyledTable>
<StyledTable css={{ /* can also style here now */ }}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you

@rupin90
Copy link
Contributor Author

rupin90 commented Jul 21, 2022

@jacksonh - Thank you, this is PR ready

@jacksonh
Copy link
Contributor

Hey @rupin90 one thing I notice is in small there's a black outline we don't need:

Screen Shot 2022-07-21 at 15 41 05

@rupin90
Copy link
Contributor Author

rupin90 commented Jul 22, 2022

Hey @rupin90 one thing I notice is in small there's a black outline we don't need:

Screen Shot 2022-07-21 at 15 41 05

You mean the black border around the table? okay I can remove that.

@rupin90
Copy link
Contributor Author

rupin90 commented Jul 22, 2022

Hey @jacksonh - how is this?

Screen Shot 2022-07-22 at 4 25 57 PM

@jacksonh
Copy link
Contributor

This looks great, thanks. I'm going to merge it now.

@jacksonh jacksonh merged commit f2f6970 into main Jul 22, 2022
@jacksonh jacksonh deleted the 895 branch July 22, 2022 17:56
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