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.

Sign up for GitHub

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

Jump to bottom

BOM Column hiding and reordering #239

Merged
merged 23 commits into from
Jun 6, 2021
Merged

BOM Column hiding and reordering #239

merged 23 commits into from
Jun 6, 2021

Conversation

phagenlocher
Copy link
Contributor

@phagenlocher phagenlocher commented Mar 17, 2021 •
edited
Loading

For some projects, having the Footprint column is not needed and just wastes space. Thus, this patch proposes an option that allows to show/hide the footprints column.

image

Especially on smaller screens the table gets hard to read if you want to get a bigger view of the PCB.

image

However, with the Footprint column removed, it looks much nicer. The table cells also take up less space which makes it possible to view more components without scrolling.

image

@qu1ck
Copy link
Member

qu1ck commented Mar 18, 2021

I will not merge this as is because the scope is too narrow. There is a related wishlist item to make all hardcoded columns configurable and reorderable. See also discussion in #167.

If you'd like to work with me on this here is the direction I can suggest:
Make a small popup menu appear on hover in the currently unused block at top left corner of the bom table. That menu should have all columns with ability to toggle their visibility and reorder them by drag and drop (preferred) or select and up/down buttons (probably easier to implement, clunkier to use). The first column (row number) and checkbox columns are special case here and the first one probably should just be toggleable but not reorderable while latter should be treated as a single block.
The state should also be preserved in localstorage, as with other settings.

@phagenlocher
Copy link
Contributor Author

I dont think that reordering inside a pop-up menu makes for a good user experience. Why not have a drag and drop reordering on the column header and use the pop up purely for visibility checkboxes?

Should reordering of the checkbox columns then also change the order of the "Bom checkboxes" option in the preferences pop-up?

@qu1ck
Copy link
Member

qu1ck commented Mar 18, 2021

Why not have a drag and drop reordering on the column header and use the pop up purely for visibility checkboxes?

Agreed, that's even better.

Should reordering of the checkbox columns then also change the order of the "Bom checkboxes" option in the preferences pop-up?

Either treat them as a single block and rely on ordering in preferences pop-up or if they will be movable individually then list in preferences should be updated accordingly.

@qu1ck
Copy link
Member

qu1ck commented Mar 20, 2021

Some notes on implementation:

  • The column hiding/reordering needs to support extra fields
  • It's best to store the data in 2 generic fields: hiddenColumns list and columnOrder list instead of hardcoding visibility toggle and storage for each column.

@phagenlocher
Copy link
Contributor Author

Agreed. I'll refactor the code and mark this as WIP.

@phagenlocher phagenlocher changed the title Option to hide footprints in BOM [WIP] BOM Column hiding and reordering Mar 22, 2021
@phagenlocher
Copy link
Contributor Author

Columns can now be reordered with drag'n'drop on the column headers. Checkboxes and extra fields are treated as a single column.

hiddenColumns and columnOrder have been added to the settings. The drag'n'drop handles the change of columnOrder, hiddenColumns is changed by the pop-up menu, which still uses the cog symbol (change that?).

When the visible columns are reordered, the hidden ones are automatically pushed to the right in the settings.

@qu1ck
Copy link
Member

qu1ck commented Apr 4, 2021

I played around with it a bit and it works but there are some issues with dark theme and column placement detection logic. I have to drag waaay to the left for it to work.

https://i.imgur.com/6eQxwNM.gif

Also I'd like to see following changed:

  • extra fields should be treated same as normal columns, i.e. not a single block. In the future difference between them in html will be minimal when currently hardcoded columns will also be optional.
  • Checkboxes in hide columns menu should be generated dynamically and their order should match table order
  • Not sure about shifting hidden columns to the right when reordering happens. It shouldn't be much harder to not do that and keep their place in the array. If you drag a column over a hidden columns place then it doesn't matter which way you choose to place the hidden one but it should still keep it's relative position to other columns.

For the icon use this

background-image: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='24' height='24'%3E%3Cpath fill='none' stroke='%23333' d='M2.5 4.5h5v15h-5zM9.5 4.5h5v15h-5zM16.5 4.5h5v15h-5z'/%3E%3C/svg%3E");

And best place for it is in top left corner of the bom table in the unused cell of the header.

@phagenlocher
Copy link
Contributor Author

Dragging should work now.

If all hardcoded columns are at some point treated as optional there has to be a standard way of naming them. How should that look? Lowercasing the header name?

I don't like the idea of elements in a menu changing their order when columns in a table get reordered, even though they are functionally linked. It seems counterintuitive to me.

Pushing all hidden columns to the right was the easiest way of implementing it. I didn't put too much thought into it since I don't suspect users to reorder columns very often.

@qu1ck
Copy link
Member

qu1ck commented Apr 4, 2021

If all hardcoded columns are at some point treated as optional there has to be a standard way of naming them. How should that look? Lowercasing the header name?

Use header name as is.

I don't like the idea of elements in a menu changing their order when columns in a table get reordered, even though they are functionally linked. It seems counterintuitive to me.

Why? It's more counterintuitive to me that if I move last column to be first it's still last in visibility menu. If you have a bunch of columns with extra fields navigating the list will be much easier if it corresponds to order in the table.

One more thing I noticed: search field still matches hidden columns. Ref lookup field should probably be disabled when references column is hidden.

@qu1ck
Copy link
Member

qu1ck commented Apr 4, 2021 •
edited
Loading

To clarify the standard column naming; they will be still hardcoded but in python part, not in html.
In html instead of config.extra_fields there will be config.fields and bom table will be a flat array of fields corresponding to config.fields. The standard columns will still get some special treatment for ref lookup, smart value sorting and similar edge cases but that will only be in effect if columns are present in config.fields.

This is just my description of future plans, I don't expect this work to be done in this MR.

@phagenlocher
Copy link
Contributor Author

The visibility menu button has now moved to the free space in the table header and is dynamically built from the fields in the order they appear in the table. Reflookup is disabled and cleared on hiding the references column. The filter now ignores hidden columns.

@qu1ck
Copy link
Member

qu1ck commented May 24, 2021

Works pretty well. Aside from cosmetic stuff I mentioned inline only 2 things left to address:

  • Extra fields to be treated individually
  • Relative order of hidden columns to be preserved when dragging (as opposed to be shifted all the way to the right)

@phagenlocher
Copy link
Contributor Author

Extra fields are now treated as normal columns. Also, the order of hidden columns is now preserved. The code should also work once all columns are generalized. The th HTML elements now have a "col_name" attribute which identifies columns by name for the drag'n'drop.

@qu1ck
Copy link
Member

qu1ck commented May 27, 2021

Please address inline comments I made earlier and remove WIP when this is ready for final review.

@phagenlocher
Copy link
Contributor Author

Which comments are you referring to?

qu1ck
qu1ck reviewed May 27, 2021
View reviewed changes
Copy link
Member

@qu1ck qu1ck left a comment

Choose a reason for hiding this comment

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

Oops, looks like I didn't submit them, sorry.

InteractiveHtmlBom/web/ibom.css Outdated
@@ -241,7 +241,7 @@ canvas:active {
}

.bom .numCol {
width: 25px;
width: 46px;
Copy link
Member

Choose a reason for hiding this comment

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

Too wide. No need to enlarge icon, this should be 30px tops.

InteractiveHtmlBom/web/ibom.css Outdated
@@ -392,7 +392,7 @@ canvas:active {
}

.filter {
width: calc(60% - 64px);
width: calc(60% - 110px);
Copy link
Member

Choose a reason for hiding this comment

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

Revert since the menu is not in filter row anymore

InteractiveHtmlBom/web/ibom.css Outdated
background-image: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='24' height='24'%3E%3Cpath fill='none' stroke='%23333' d='M2.5 4.5h5v15h-5zM9.5 4.5h5v15h-5zM16.5 4.5h5v15h-5z'/%3E%3C/svg%3E");
background-position: center;
background-repeat: no-repeat;
background-size: 30px 30px;
Copy link
Member

Choose a reason for hiding this comment

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

Why enlarge the icon? Keep it original size, I pixel aligned the svg for crispyness.
Also override size from button class, it's too large.

InteractiveHtmlBom/web/ibom.css Show resolved Hide resolved
InteractiveHtmlBom/web/ibom.css Outdated
@@ -445,6 +445,24 @@ mark.highlight {
background-repeat: no-repeat;
}

.visbtn {
background-color: transparent;
Copy link
Member

Choose a reason for hiding this comment

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

Doesnt work with dark theme, I dont think it's needed.

InteractiveHtmlBom/web/ibom.js Show resolved Hide resolved
InteractiveHtmlBom/web/ibom.js

vismenu.appendChild(visbutton);
if (settings.bommode != "netlist")
vismenu.appendChild(viscontent);
Copy link
Member

Choose a reason for hiding this comment

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

If it is netlist mode, hide the icon itself too

InteractiveHtmlBom/web/ibom.js Show resolved Hide resolved
InteractiveHtmlBom/web/util.js Outdated
Comment on lines 410 to 419
showFootprints(settings.show_footprints);
document.getElementById("showFootprintsCheckbox").checked = settings.show_footprints;
setShowQuantities(settings.show_quantities);
document.getElementById("showQuantittiesCheckbox").checked = settings.show_quantities;
setShowCheckboxfields(settings.show_checkboxfields);
document.getElementById("showCheckboxfieldsCheckbox").checked = settings.show_checkboxfields;
setShowCheckboxfields(settings.show_references);
document.getElementById("showReferencesCheckbox").checked = settings.show_references;
setShowValues(settings.show_values);
document.getElementById("showValuesCheckbox").checked = settings.show_values;
Copy link
Member

Choose a reason for hiding this comment

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

Out of date

InteractiveHtmlBom/web/util.js Outdated
@@ -441,6 +451,7 @@ var settings = {
darkMode: false,
highlightpin1: false,
redrawOnDrag: true,
show_footprints: true,
Copy link
Member

Choose a reason for hiding this comment

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

out of date

@phagenlocher
Copy link
Contributor Author

This should address all comments. I could not find a nicer way around overwriting the font-family in #vismenu-content again, since .bom overwrites it. However, I consider this ready for final review.

@phagenlocher phagenlocher changed the title [WIP] BOM Column hiding and reordering BOM Column hiding and reordering Jun 5, 2021
@qu1ck qu1ck merged commit 8c4ce8f into openscopeproject:master Jun 6, 2021
@qu1ck
Copy link
Member

qu1ck commented Jun 6, 2021

There are some minor css and formatting issues but I'll take care of it a bit later.

Thanks for your contribution, this is a pretty neat feature!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qu1ck qu1ck qu1ck left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@phagenlocher @qu1ck