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.

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

fix(UI - layer_list_panel): Change layer number width spacing to consider archived layers #748

Merged
merged 6 commits into from
Mar 5, 2025

Conversation

seankmartin
Copy link
Contributor

The layer list panel number width is determined by the number of characters in the highest number non-archived layer. Since archived layers didn't contribute to the layer count for determining the width for the layer list panel numbers, it could result in cases like this where the number would overlap the visibility icon for archived layers:

image

We're thinking to change the count here to take all layers into account so that archived layer numbers don't overlap the visibility icon and be like this:

image

Maybe there is some reason though that archived layers were previously excluded for the number width count that we should be considering here? For example, maybe the layer numbers were intended to be hidden for all archived layers. If so, happy to amend this to account for that, thanks

@jbms
Copy link
Collaborator

jbms commented Mar 5, 2025

Thanks for spotting this issue.

The text-align: right was intentional so that the numbers align more nicely.

I did intend not to display numbers for achived layers, because the numbers are mainly useful for the key bindings that toggle layer visibility: 728c40f

However, that was accidentally broken by this commit: ffbb538
Since that assigns a "nonArchivedLayerIndex" even to archived layers, the original condition for hiding the numbers did not work.

So one fix would be to just fix the condition in layer_list_panel.ts to check layer.archived rather than if the index is -1.

However, it is also a bit unfortunate to display archived layer numbers for tool bindings in the top bar, but then never actually indicate which layer that number corresponds to.

For now, though, I think it would be best to just fix the logic for hiding the number.

@seankmartin
Copy link
Contributor Author

That makes sense, thank you for the context Jeremy!

We will update this one to fix the logic for hiding the number in layer_list_panel.ts instead of using the archived layer number for the width calculation and remove the other changes here

@seankmartin
Copy link
Contributor Author

Changed the check now for hiding numbers of archived layers in the layer_list_panel.

Could be missing again some reason for the old width calculation, but updated that too to directly use the number of characters in the string representation of the number of non-archived layers, not the number of non-archived layers + 1

@jbms
Copy link
Collaborator

jbms commented Mar 5, 2025

Thanks

Yeah, the +1 was a bug I guess --- I added that because it is numbered from 1, but that is already taken into account...

@jbms jbms merged commit 7681ed7 into google:master Mar 5, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants