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

Add cross axis mapping panel #1729

Merged
merged 52 commits into from
Oct 23, 2024
Merged

Conversation

ollimeier
Copy link
Collaborator

Fixes #1657

I guess this is good enough for a first draft PR to get feedback.

Note:

There are some known issue:

  • Reordering a card shows a wrong height of the card if closed (maybe a good follow up, because I am not sure if this should be fixed within setupSortableList)
  • when editing a cross-axis mapping, the UI gets reloaded. This is not a big deal, BUT if one or more cards are closed, it's irritating, because these closed cards will be open afterwards.

@ollimeier
Copy link
Collaborator Author

Here a screenshot of how it looks currently:
Bildschirmfoto 2024-10-16 um 16 38 42

And this is not a mockup anymore ;)

@justvanrossum
Copy link
Collaborator

Here a screenshot of how it looks currently

Great!

Some visual feedback:

  • Please reduce the space between the slider and its checkbox (could be similar to the space between the numeric field and the slider). This should make it clearer that the checkbox belongs to the slider to its left
  • Let's leave out the "incl." label (if could get a tooltip, like I suggested earlier)
  • Please increase the space between the "Input/output location" label and the sliders
  • The axis label should be axis.name, not axis.tag (see the Axes panel, but also the designspace-navigation panel)

Behavior:

  • When the slider gets moved back to the default, this should not imply unchecking the checkbox. The checkbox is needed because there is not a 1-on-1 relationship between "axis is at default" and "axis participates".

Question:

  • Undo doesn't seem to work?

when editing a cross-axis mapping, the UI gets reloaded. This is not a big deal, BUT if one or more cards are closed, it's irritating, because these closed cards will be open afterwards.

That actually does make it a big deal. Why is it necessary to reload the UI on every change?

@justvanrossum
Copy link
Collaborator

Layout structure:

I'm not confortable with reusing designspace-location this way, and the changes to the designspace-location web-component. The labels and checkboxes now have their own grid, and it will be hard to keep this perfectly in sync with the sliders grid. It should be a single grid:

+-----------+----------------+----------+-----------------+----------+
|           | Input location            | Output location            |
+-----------+----------------+----------+-----------------+----------+
| axis name | range-slider   | checkbox | range-slider    | checkbox |
+-----------+----------------+----------+-----------------+----------+
| axis name | range-slider   | checkbox | range-slider    | checkbox |
+-----------+----------------+----------+-----------------+----------+
| etc.      |                |          |                 |          |

@ollimeier
Copy link
Collaborator Author

+-----------+----------------+----------+-----------------+----------+
| | Input location | Output location |
+-----------+----------------+----------+-----------------+----------+
| axis name | range-slider | checkbox | range-slider | checkbox |
+-----------+----------------+----------+-----------------+----------+
| axis name | range-slider | checkbox | range-slider | checkbox |
+-----------+----------------+----------+-----------------+----------+
| etc. | | | | |

mmmhhh, I am not sure if I understand this correctly.
Currently it looks like this:

+------+-----------+-----------------+----------+-----------------+----------+------+
| Icon |           | Description     |          | Group Descript. |          | Icon |
+------+-----------+-----------------+----------+-----------------+----------+------+
|      |           | Input location  |          | Output location |          |      |
+------+-----------+-----------------+----------+-----------------+----------+------+
|      | axis name | number | slider | checkbox | number | slider | checkbox |      |
+------+-----------+-----------------+----------+-----------------+----------+------+
|      | axis name | number | slider | checkbox | number | slider | checkbox |      |
+------+-----------+-----------------+----------+-----------------+----------+------+
| etc. |           |                 |          |                 |          |      |

I don't fully understand how it should look like. When we want to keep the same look and feeling. Sorry.

@ollimeier
Copy link
Collaborator Author

In todays Meeting we discussed to refactor the two areas 'Applications settings' and 'Font Info' to reuse same code parts. This is a follow up. Prio has not been discussed, yet.

@ollimeier
Copy link
Collaborator Author

ollimeier commented Oct 17, 2024

We also discussed an idea to have one or two preview glyphs for each card to get a visual representation of what we specify with input and output location. Should be a follow up.

@ollimeier
Copy link
Collaborator Author

Hi @justvanrossum, I will set this PR to 'Ready for review' in a bit.

I was able to address all feedback:

  • reduce the space between the slider and its checkbox -> done.
  • leave out the "incl." -> done.
  • increase the space between the "Input/output location" label and the sliders -> done.
  • axis label should be axis.name, not axis.tag -> done.
  • moving slider back to default, should not imply unchecking the checkbox -> done.
  • Undo doesn't seem to work? -> works now. done.
  • any editing caused a reload of the UI -> not anymore. done.
  • don't use designspace-location -> done.
  • folded card without input/output headlines -> done.
  • wrong draggable card height if a card is folded -> works now.
  • remove 'Unnamed' string for new added mapping -> done.
  • fix tooltip for checkboxes -> done.

@ollimeier ollimeier marked this pull request as ready for review October 17, 2024 14:47
@justvanrossum
Copy link
Collaborator

Great, thank you!

@justvanrossum
Copy link
Collaborator

I think found a bug: if you move a slider that is not yet participating once, then

  1. the default value (or 0?) is written to the file instead of the slider value
  2. undo seems to contain an empty step? you have to undo twice before the undo takes place

(FWIW, this is not related to the designspace backend: if I edit a .fontra project, the same thing happens)

@justvanrossum
Copy link
Collaborator

Request: please swap the Group description with the Description, so the Group description comes first.

@ollimeier ollimeier force-pushed the issue-1657-add-cross-axis-mapping branch from 0c148de to 3c23e20 Compare October 22, 2024 05:40
@ollimeier
Copy link
Collaborator Author

Request: please swap the Group description with the Description, so the Group description comes first.

Agree. I changed it.

@ollimeier
Copy link
Collaborator Author

I think found a bug: if you move a slider that is not yet participating once, then

  1. the default value (or 0?) is written to the file instead of the slider value
  2. undo seems to contain an empty step? you have to undo twice before the undo takes place

(FWIW, this is not related to the designspace backend: if I edit a .fontra project, the same thing happens)

Thanks a lot for finding this. I can reproduce it. I'll have a look.

@ollimeier
Copy link
Collaborator Author

I think found a bug: if you move a slider that is not yet participating once, then

  1. the default value (or 0?) is written to the file instead of the slider value
  2. undo seems to contain an empty step? you have to undo twice before the undo takes place

(FWIW, this is not related to the designspace backend: if I edit a .fontra project, the same thing happens)

Thanks once again for finding these issues. They were related to each other. The last commit fixes both.

@ollimeier
Copy link
Collaborator Author

@justvanrossum I noticed, that the value written to the file has quite a lot of digits after the decimal point:
Screenshot 2024-10-22 at 09 12 21

Should we limit this to two?

@justvanrossum
Copy link
Collaborator

Should we limit this to two?

I don't think it's a problem, let's keep the digits.

@justvanrossum
Copy link
Collaborator

It seems the width of the slider column seems to depend on the with of the description label:

image

It would be better if the sliders had the same widths.

@ollimeier
Copy link
Collaborator Author

It seems the width of the slider column seems to depend on the with of the description label:

image It would be better if the sliders had the same widths.

Once you see it, you cannot unsee it – right?! Thanks for finding this. I simply have not seen it till now. I made it minmax(200px, 300px).

@ollimeier
Copy link
Collaborator Author

While editing the width of the column, I was wondering if it would make sense to use 'Group' instead of 'Group description' for the label, because 'Group description' is quite long. Any thoughts about that?

@justvanrossum
Copy link
Collaborator

use 'Group' instead of 'Group description'

Hm, not sure. Let me think about it. The whole thing is a bit weird in the .designspace format.

What helps a little bit is to change the grid-template-columns to min-content auto in .fontra-ui-font-info-cross-axis-mapping-panel-column: that keeps the space for the label to a minimum.

At the same time, the text entry box should be allowed to go over the checkbox columns, so I think each description label+input could be two columns wide.

@justvanrossum
Copy link
Collaborator

With a narrow viewport, the left text input bleeds into the next column instead of shrinking:

image

Perhaps the label should also be allowed to shrink.

@justvanrossum
Copy link
Collaborator

The label + input baseline should be aligned:
image

@justvanrossum
Copy link
Collaborator

image

This phrasing is awkward, but I'm also not sure what this tooltip is helping at all. Maybe remove it? But if you want to keep it, it would be slightly better as "Specify axes and their values for the input location". (Same for output.)

image

Suggest: "When checked, this axis participates in the mapping"

@ollimeier
Copy link
Collaborator Author

image This phrasing is awkward, but I'm also not sure what this tooltip is helping at all. Maybe remove it? But if you want to keep it, it would be slightly better as "Specify axes and their values for the input location". (Same for output.) image Suggest: "When checked, this axis participates in the mapping"

Thanks. I removed and change it.

@ollimeier
Copy link
Collaborator Author

The label + input baseline should be aligned: image

Thanks. That's weird because I use labeledTextInput I wouldn't expect to have to touch it.

@ollimeier
Copy link
Collaborator Author

The label + input baseline should be aligned: image

Thanks. That's weird because I use labeledTextInput I wouldn't expect to have to touch it.

display: grid; changes the visual position of the baseline :(

ollimeier and others added 24 commits October 23, 2024 16:49
…lue + undo twice before the undo takes place
@ollimeier ollimeier force-pushed the issue-1657-add-cross-axis-mapping branch from 98fa204 to b94c733 Compare October 23, 2024 14:51
@ollimeier
Copy link
Collaborator Author

I think this is now good to go. Let's merge! 🎉

Sadly not. There is still a visual bug:
Screenshot 2024-10-23 at 16 53 20

@ollimeier
Copy link
Collaborator Author

I think this is now good to go. Let's merge! 🎉

Sadly not. There is still a visual bug: Screenshot 2024-10-23 at 16 53 20

I fixed it. I will merge it now :)

@ollimeier ollimeier merged commit 058ec9b into main Oct 23, 2024
5 checks passed
@ollimeier ollimeier deleted the issue-1657-add-cross-axis-mapping branch October 23, 2024 15:08
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.

[avar-2] Design, implement and test basic authoring UI
2 participants