-
Notifications
You must be signed in to change notification settings - Fork 198
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
Unify table on the home page #1065
base: main
Are you sure you want to change the base?
Conversation
Great! I think the main functionality is all there and behaves as expected. If one deselects a complete group of column (disabling one row of columns in the "Select Columns" view), I think it should be hidden completely from the summary view. Right now the cells in the header rows remain. |
I have added the handling for the same as well! If a runset's all columns are hidden, now it does not render the column related to the same, and also shows a message at the end of the table (sort of a warning). Do you think we should keep the warning? |
PS. I just noticed that all works well and good right now if we hide the first runset completely, but some of the data is not displayed properly when the second column is completely hidden. Can you please point me to the appropriate function responsible for calculating these tableHeaders and colspans, so that I can make use of the same again to calculate the same correctly whenever the hidden runsets changes? |
I think some visual indicator of hidden columns and run sets is good to have, yes. Maybe not like a warning below but something in the table? I think in the main table the column borders remain visible when a column is hidden, so if you hide more columns you get thicker borders. Or some icon or so? Not sure, but we can iterate on this.
Hm, didn't see this right now.
Do you mean the colspan in what is currently the benchmark setup table? Everything in that table is produced here, and colspan is called "width" there. You can see the result of this function in the JSON code that is written by table-generator into the HTML template. |
Maybe instead of removing columns, just set there width to 0 or make them otherwise invisible? Then you would not have to worry about colspan. |
Got it, sir! Having an info icon on the first row with a tooltip might be a good way to communicate the same. I have added that in the latest push. You can let me know how that looks and feels. Also the idea of setting the width to 0 worked like a charm, and avoided me having to go into all of the python code regarding the calculations involving the spans! Thanks. |
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.
Yes, the info icon seems like a good idea, I like it.
Hiding the first runset breaks the content of the rows with limits etc. for me. Do you see this as well?
Except for this, I think functionality is fine! 👍 We can start thinking about polishing the UI design already. I have some suggestions for this, but if you think you have a better or alternative idea, I am completely open and would like to hear your ideas!
The style of "Click here to select columns" is different on the Summary and the Table tab, maybe needs some CSS adjustment?
The "Fixed row title" should probably be aligned to the left such that it is above the fixed column. But otherwise it is probably fine.
The heading "Benchmark Setup" does not describe it correctly anymore, maybe "Benchmark Setup & Statistics"? Or we get rid of it altogether, as most other tabs also do not have a header. We could also rename what is in the tab name, e.g., put "(Benchmark) Setup & Statistics" there?
The vertical alignment of the statistics rows and the row names in the left columns does not match for me.
On windows that are horizontally narrow and after scrolling to the right, there is no visual separator anymore between the fixed left-most column and the rest. Probably the border that is there should not scroll away.
I think I prefer the thicker borders between the run sets that were previously in the statistics table. But the borders between the column names could become thinner again. It is certainly not a bad idea if as much as possible looks similar to the table on the Table tab.
Not sure about the alternating gray backgrounds in the statistics parts of the table. They look pretty strong, are they the same gray as in the top part? Due to the fact that these statistics rows are semantically grouped into something like a tree (cf. the indentation of the row names), it might also be a good idea to keep these without alternating background colors, which in the draft makes it look more like a purely sequential list.
Maybe we can also improve the overall layout a little bit. Right now it looks a little bit like a table that has a header row in the middle (because there is this one bold row). I would think it could be good if we give some visual indication that either (a) the top half is altogether the header or that (b) there are two parts of the table, each with its own header.
For (a) maybe it is enough to remove the bold, and add some bottom border to the row with the column names?
For (b) maybe some more vertical space in the middle or so?
But if we manage to create (a) with a nice UI I guess I would prefer (a) over (b).
@@ -5,8 +5,6 @@ | |||
// | |||
// SPDX-License-Identifier: Apache-2.0 | |||
|
|||
"use strict"; |
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.
Is removing this intentional?
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.
No sir. Will revert!
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 will be numbering the suggestions and the discussions so that it is easier to keep track of the same! 1
Glad that you liked it. Have moved it's styling to the one global stylesheet. 2
Yes sir. The problem stems from the fact that when all of the row data like the col spans and all are calculated, they are calculated to show the data in the first column. What I mean by that is, if a field is supposed to span two columns, then the return would be like I was trying to fix the same but can't seem to find a simple way to do it. The best thing would be to recalculate the stats and everything, but that is a tedious task. I tried implementing a single recalculation of the col spans algorithm in JS but dropped the idea to the overwhelming number of edge cases: like what we have, we have 3 runsets, and one column spans 1-2 while the other spans the 3rd. Now to handle the cases involving the hiding of any one or two of the columns does not remain an easy algorithm. Do you have any suggestions for the same? 3
Fixed the styling in the latest push! 4
Added styles to align to the left, and center the content of the checkbox and it's corresponding label in the latest push. 5
Makes sense! Would remove the header from the the tab, and rename the tabs to 6
Can you share a screenshot of the same along with the dimensions of your screen? There might be some styling issues, as we are currently kind of relying on the CSS to reposition the rows correctly instead of ensuring the same by semantic structure. 7
Got it sir. Trying to fix the same. 8
9
Yes, they are currently.
I get your point, but removing the same led to the UI feeling a bit incoherent and broken. As I envision it, the whole point of these stripes is to make reading the table easier so that it is easier for the user to map the title to the actual row content. Since now the statistics are even a part of a "single" table, it makes more sense to keep them from a consistency POV. These were my two cents, but if you feel that the same is not the best way to proceed with them, I can undo the alternating highlighting. 10
Got it sir. I have removed the bold text and instead added borders as a visual demarcator. Let me know how you feel above the same. |
Hm, but shouldn't that be also solved by not hiding the columns, just setting their width to zero? If I use the browser dev tools to remove
With "stats" you mean the colspan info? (Just making sure that we are not talking about the "statistics" part of the table, because logic for recomputing that already exists.)
The algorithm that we use so far is not that difficult: start with the full expanded list, then find sequences of duplicates, and count them. But maybe you don't need it. For the rest I will have a look later. |
Thanks for your work, here are my responses for the rest of the items. Everything not responded to is fine.
👍
Good. I just noticed that there doesn't really seem to be a reason why we do not style it as a regular button, does it? Could you try out how it looks if you just let the browser render it as usual? Maybe we can then even remove the weird "Click here to", if it is obvious that it is a button?
Looks nice. Maybe we can remove or reduce the vertical spacing above the table now?
The dark grey bottom border of the statistics cells looks unaligned and also the borders of the row with the column names.
Did you want to write something here?
Still undecided. Note how the main table on the table tab does not have alternating background colors. Btw: What happens if the number of rows in the top half of the table changes? In my test table the row with the column names always has a gray background, but there are some optional rows in the top half. How does it look if the number of rows changes? I guess we do want to always make the row with the column names highlighted in some way, and not let it have a white background? Maybe we need a lighter gray for the alternating background and a somewhat darker gray for the background of the row with the column names?
Yes, I think this is better than before. We just have to fine-tune, for example right now the vertical border between the run sets is brighter than the vertical borders within columns in one run set, which looks weird and contradicts the semantics (columns of one run set belong together). Can you change this, please? |
Got it, sir! Will try to make all of the recommended changes by tomorrow! |
Sorry for the delay on my end! I have been trying to improve the styling and CSS per the requirements described. However, working with design is not my forte. I have been able to get most of the issues resolved, but the current blockers are:
Other than that, all the other points seem to be addressed (if I'm not missing any). Will re-iterate through the checklist soon after these fixes. |
I have polished the UI a bit more and made the following changes:
The only challenge that seems to be left is aligning the table properly. Since we are rendering one new table for each runset in the larger main table, aligning the rows seems troublesome. I am trying to manually fix the height proportions to see if it works, but it doesn't look too promising. I would appreciate any pointers or ideas on how to solve the same. |
I found some additional request in #506 (comment): In vertically small windows, the horizontal scroll bar should be at the bottom of the browser window, and not be scrolled out of view vertically. If I remove |
Some details I noticed and features I would use:
|
Awesome! Many thanks! Everything I wanted in #506 is fulfilled! |
This would be great indeed! |
I guess this makes sense, thanks for the suggestion. @EshaanAgg But we would want to implement it separately, not in this PR. |
Done. Reverted them back to as usual.
Fixed the alignment issue as well.
Using a darker shade of grey seemed to break the consistency of the design. The best (and the simplest) UI according to me was keeping the same shade of grey for all borders, and then using thicker borders to separate runsets, and thinner ones to separate the columns of a runset. I have updated the same in the latest push. Please let me know how you feel about it.
It is automatically calculated by React Table.
Added the fix for this as well. Now we use the browser's scrollbars for horizontal scrolling, the
Fixed.
Have changed the same to a lighter tone so that the borders are still visible.
Didn't understand this. Can you please provide more context, or perhaps a screenshot?
Sure. @PhilippWendler how does this sound to you? Should I try to implement the same?
Sure. As mentioned by @PhilippWendler sir, will try to add it another PR once this is merged. |
I think they are still thicker than before?
Ok. Maybe let's try with making all of them smaller? For example same width as before.
Ah, so it is caused by the contents of all the cells. Well, I guess this makes sense.
Thanks. I guess we could make
I guess what is meant is that there is simply not much space between the numbers in the right most cells and the window border (for narrow windows). But it seems this was the same before, so let's keep it for now.
Sure, I actually do not remember why we did not have it. I guess it should look just like in the main table. Ideally we would also always use the same color for all hover effects (and define it in a CSS variable). |
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 a lot! Here are a few minor things I found while clicking through the current state.
There is now more whitespace to the left of the text in the "options" cells. I think this only wastes space. Please try to keep the changes to the style minimal, i.e., change only what is now required due to the restructuring but keep everything else the same.
The column headers ("status" etc.) are clickable, but this is only indicated by the cursor style. Previously we also had a hover effect, can we re-add this?
The left-most column seems to no longer be resizable. Can this feature be restored?
I created a table where a line break happened in the left-most column occurred, and this messed up the alignment:
Can you please have a look?
Should we maybe move the info icon for hidden run sets to the cell where the "Select Columns" button is? On the one hand it seems good to have it at the top, on the other hand it fits well next to "Select Columns".
The introduction of the footer on the plot tabs seems to have lead to double vertical scroll bars. Having the footer is maybe nice, but then the scroll bars need to be fixed.
className="infoTooltipContainer" | ||
onMouseEnter={(e) => { | ||
const tooltip = e.currentTarget.querySelector(".infoTooltip"); | ||
tooltip.style.visibility = "visible"; |
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 would prefer to have less CSS in the JavaScript code. I think usually we handle visibility of DOM elements via CSS classes, can we do that here as well?
column={column} | ||
className="header-data clickable" | ||
title="Show Quantile Plot of this column" | ||
style={{ |
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.
Please move CSS to the stylesheets (also below). Not only is it more manageable there, but it also reduces the size of our snapshot tests.
Hey! I have completed most of these required changes and will push the code soon! The only bottleneck is the layout breaking when the content of one of the rows spans more than one line. I have been trying to dynamically match the heights of the corresponding rows in the main table and the child statistics tables, but the same is causing multiple rerenders in a second, to the extend it causes blocking of the UI and the browser hanging. Still trying to debug the same. |
I have added the basic functionality related to merging the UI of the two tables in the Overview component. I would highly appreciate it if @PhilippWendler, sir you could check if the table is performing as expected in terms of allowing the users to:
After the same is completed, I would work on improving the UI as per the suggestions, and then would update the test suite as well as the build.
Fixes: #506