-
Notifications
You must be signed in to change notification settings - Fork 2
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
[NM-59] Add input comparison table #1023
Conversation
@@ -0,0 +1,35 @@ | |||
<template> | |||
<span class="d-inline-flex"> | |||
<!--Setting a transform to push icon up 1 pixel to align slightly |
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 was fighting with this a bit, not sure what it looks like on your guys end? Just looks like to me the actual character ▲has more white space above than below and so is rendering kind of ugly on firefox. Couldn't really work out a nice way to make it align.
Actually looking at the screenshots now it looks like from chromium perhaps I didn't need to push it up a pixel and that chromium is displaying it nicely.
I tried looking to see if there was an appropriate vue-feather or font-awesome icon I could use instead which might be easier to centre. But vue-feathers was a big rounded triangle so not quite right and all the similar looking ones on font-awesome were premium.
Also considered just drawing svg path in manually but that seemed like madness.
Curious on your thoughts and how you would solve this?
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.
hmm so for me, this is what it looks like on chrome:
the spacing seems alright to me right now, the size of the icon however seems different between the two
theres ways to target css to specific browsers, for example can target only firefox with this: https://stackoverflow.com/questions/952861/targeting-only-firefox-with-css
would recommend stuff like that if it is really needed
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.
spacing looks ok to me on both Firefox and Chrome; on my end I am seeing the same icon size in both browsers, the larger size that Mantra showed in the second screenshot
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.
If consistent sizing is an issue and you do end up deciding to replace with an icon, looks like the "play" icon from vue-feather might be suitable. Could do a 90 degree rotation in either direction to get it oriented correctly.
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.
Thanks! Rotated play icon is a great idea. If this looks ok to you both then let's go with it, if we see some issues down the line we can switch to the play button icon. I don't mind the difference in size on linux from chrome to firefox, as long as they are centered :D
|
||
const defaultColDef = { | ||
// Set the default filter type | ||
filter: 'agNumberColumnFilter', | ||
// Floating filter adds the dedicated row for filtering at the bottom | ||
floatingFilter: true, | ||
floatingFilter: false, |
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.
Chatted with Rachel about this, she thinks turn it off for now to save some vertical height. We can put it back on if people keep missing them
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.
yh think this looks slicker without it
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.
yh think this looks slicker without it
@@ -38,11 +39,14 @@ const defaultColDef = { | |||
// Stop the columns from being draggable to rearrange order or remove them | |||
suppressMovable: true, | |||
}; | |||
const ROW_HEIGHT = 35; |
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 reduced the row height here too, think it looks a little better
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.
definitely agree
/update-snapshots |
Description
This PR will add a table for showing the input comparison data. Should be reviewed and merged after #1022
There are a couple of general features this has added
Other features
Next steps
Type of version change
Minor
Checklist