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

query parameter to show position regions #409

Closed
zepumph opened this issue Nov 10, 2021 · 13 comments
Closed

query parameter to show position regions #409

zepumph opened this issue Nov 10, 2021 · 13 comments

Comments

@zepumph
Copy link
Member

zepumph commented Nov 10, 2021

Like we did in GFL and BASE

This is just the general regions when there are no tick marks.

@zepumph
Copy link
Member Author

zepumph commented Nov 10, 2021

@terracoda please review by adding ?showRegions to a version of RAP.

It looks like:

image

@zepumph
Copy link
Member Author

zepumph commented Nov 10, 2021

@terracoda
Copy link
Contributor

Looks good. This should work for me.

Just one fix... I think "at middle" is a single value region, so the line should probably go through the middle of the words like "at top" and "at bottom".

@terracoda
Copy link
Contributor

I'll assign back as soon as I determine the new region ranges.

@zepumph
Copy link
Member Author

zepumph commented Nov 11, 2021

Oh no! I just realized that I input the incorrect values for the ranges, I used the distance ranges instead of the position ones. One fix coming in up.

@zepumph
Copy link
Member Author

zepumph commented Nov 11, 2021

image

@zepumph
Copy link
Member Author

zepumph commented Nov 11, 2021

@terracoda I'm so terribly sorry for wasting your time. Please re-review.

@terracoda
Copy link
Contributor

From the screenshot it does not look like "at middle" or "at top" are single-value regions.

@zepumph
Copy link
Member Author

zepumph commented Nov 11, 2021

Maybe more like this?

image

@terracoda
Copy link
Contributor

terracoda commented Nov 12, 2021

Ok, I think you got the original PDOM regions now! That's nice!
It might be better to call the query parameter "showVoicingRegions" instead or have two parameters, though I think I only need one for Voicing. I don't want to change the regions for Interactive description.

I am wondering for Voicing if we can change the ranges for the 3 main regions around the middle to eliminate the dashed regions, "in upper-middle region" and "in lower-middle" region.

I worked the following regions out in a table in the design doc:
Voicing Regions - Version 2
= 10 {{at top}}
9.00 - 9.99 {{near top}}
6.01 - 8.99 {{in upper region}}
5.01 - 6.00{{in middle region}}
= 5.0 {{at middle}}
4.00 - 4.99 {{in middle region}}
1.01 - 3.99 {{in lower region}}
0.01 - 1.00 {{near bottom}}
= 0 {{at bottom}}

@zepumph
Copy link
Member Author

zepumph commented Mar 2, 2022

It might be better to call the query parameter "showVoicingRegions" instead or have two parameters, though I think I only need one for Voicing. I don't want to change the regions for Interactive description.

I am wondering for Voicing if we can change the ranges for the 3 main regions around the middle to eliminate the dashed regions, "in upper-middle region" and "in lower-middle" region.

Over in #433 it is very unclear if we are actually going to go through with this work. If we do, it will happen in that issue, and we can consider how it will effect this query parameter there.

I did rename it to showPositionRegions though; it was something I was wanting to do for quite a while.

After #437 this is what the position regions layer now looks like:
image

@terracoda, is there anything else for this issue?

@zepumph zepumph removed their assignment Mar 2, 2022
@terracoda
Copy link
Contributor

I don't think so. I don't think we will change the regions after all, except the change we already made around the middle, ... and now that I re-look at my table, I think I may have been trying to address that edge case with the simplified regions.

Anyways, changing the regions the least amount as possible is good in the long run.

@terracoda
Copy link
Contributor

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants