-
Notifications
You must be signed in to change notification settings - Fork 3
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
Search improvements #154
Search improvements #154
Conversation
fa63d88
to
91cc963
Compare
Deploying 2nicove with
|
Latest commit: |
a4ba4ff
|
Status: | ✅ Deploy successful! |
Preview URL: | https://7458faa4.unicove2.pages.dev |
Branch Preview URL: | https://search-history.unicove2.pages.dev |
91cc963
to
7c9d828
Compare
ae5c848
to
a4ba4ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason to get rid of the table component? I saw that you added the global css, but I didn't mind using a Table component 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the native table element will be more familiar for most devs 🤔 My initial push back was because I always prefer not adding global css when possible, but maybe it's fine in this situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The component pieces were mainly just applying styles right now. To get things like props, classes, onclick
methods, and especially melt-ui
builder functions to work it involved adding a lot of complexity with a layer of indirection that I felt wasn't needed.
Additionally, once I started needing "table styles" that weren't on a table
element I would need to duplicate the table styles on another component and keep them in sync... Once that happened, it made sense to extract the table styles into a reusable class that could be applied on a <tr>
or a <li>
or <div class='subgrid'>
or whatever.
And at that point, the <Table.Row>
component was essentially just a <tr>
anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Styles are not automatically applied on all tables globally. A class is used on the root <table>
to turn them on for all elements in table. And you can apply styles for just the row, or just the header, or just the hover effect (for example) which is what I needed in the search history component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's fine and like you said we can always go back to having a Table component if we need to in the future 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💪
$lib/utils
with barrel exportindex.ts