-
-
Notifications
You must be signed in to change notification settings - Fork 311
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
Add new column types #1223
Add new column types #1223
Conversation
Your Render PR Server URL is https://toolpad-pr-1223.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cddc3kcgqg4f13hl17j0. |
if (IMAGE_REGEX.test(value)) { | ||
return 'image'; | ||
} | ||
if (URL_REGEX.test(value)) { |
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.
Just an idea - we could also probably try to use native URL
construct - seems that when we pass invalid url it throws an error. One benefit of that is that we wouldn't need to "own" regexp in our code:
try {
new URL(value)
return 'link'
} catch (error) {}
Maybe it's not needed right now, but it might be worth to think if it wouldn't be useful to have these methods defined somewhere in utils
section, I believe we might also want to reuse this logic in the future for other components 🤔
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.
👍 Always use URL
to parse urls: https://github.com/mui/mui-toolpad/blob/8e6d211d2beb4a346a4dd153105f5190b8a842f7/packages/toolpad-app/src/utils/strings.ts#L56
When you read a url parsing regex on stackoverflow, it will most likely be wrong.
Same for testing the images, parse the pathname
out, get the extension and compare with a list. Perhaps we can add a getExtension(path: string): string
?
The regex incorrectly matches input like
'https://www.example.com/no-an-image?value=dgha.pngthgfdaghg'
'https://images.png.example.com/'
'this is a url: https://www.exaple.com/my-image.png'
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 didn't mean for these regexes to be perfect as they're used just to infer column types, but those suggestions are good, I didn't know about using the URL
constructor. Will try it out!
@@ -337,6 +369,7 @@ const DataGridComponent = React.forwardRef(function DataGridComponent( | |||
message: typeof errorProp === 'string' ? errorProp : errorProp?.message, | |||
}, | |||
}} | |||
{...(hasImageColumns ? { getRowHeight: () => 'auto' } : {})} |
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.
from https://mui.com/x/react-data-grid/row-height/#variable-row-height
Always memoize the function provided to
getRowHeight
. The grid bases on the referential value of these props to cache their values and optimize the rendering.
Co-authored-by: Vytautas Butkus <vytautas.butkus@gmail.com> Signed-off-by: Pedro Ferreira <10789765+apedroferreira@users.noreply.github.com>
@@ -315,6 +344,13 @@ const DataGridComponent = React.forwardRef(function DataGridComponent( | |||
[getRowId, columns], | |||
); | |||
|
|||
const handleGetRowHeight = React.useCallback(() => { |
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.
@apedroferreira This is recalculating hasImageColumns
for every call to getRowHeight
. This could lead to O(nm) complexity, we can turn this O(1) if we do the following:
const getRowHeight = React.useMemo(() => {
const hasImageColumns = columns.some(
({ customType }: ToolpadGridColDef) => customType === 'image',
);
return hasImageColumns ? () => 'auto' : undefined;
}, [columns])
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.
Ah, I did not think of that, good point.
I'll add a fix in a separate PR.
Closes #827
Add new Link and Image column types.
Link:
Screen.Recording.2022-10-27.at.18.31.17.mov
Image:
Screen.Recording.2022-10-27.at.18.34.38.mov
: