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

SVG view improvements #87

Merged
merged 5 commits into from
Feb 4, 2024
Merged

SVG view improvements #87

merged 5 commits into from
Feb 4, 2024

Conversation

Ced-C
Copy link
Contributor

@Ced-C Ced-C commented Feb 4, 2024

Improving svg classical deadkeys: removing * prefix and setting they in red.
Adding a base class on css to dispaly Alpha keys and deadkey layer.

Copy link
Collaborator

@fabi1cazenave fabi1cazenave left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. It’s simple and efficient, I like that a lot. :-)

It works as is, but I’ve let a couple nitpicks that you may address if you will in order to further improve the code readability and expressiveness.

/* Adding Base class option to display Alpha + dk layers */
.base .level3, .base .level4 { display: none;}
.base .level5 { opacity: 0.5; display: block;}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I’m not sure whether I prefer 1dk over base for this class name, but base is consistent with the TOML layer description, indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Up to you,
I think the dk already exists, and show all level 5 and 6.
I think that the layout is better described with alpha and a trimed dk layer for a quick overview of the layout.
In fact, I find it more useful on Ergo-L / Bépolar type of layout than the alpha+AltGr that is output by default.

kalamine/layout.py Outdated Show resolved Hide resolved
if dead_char_previous := main_deadkey.get(chars[0]):
if upper_key(dead_char_previous) == dead_char:
# Do not print letters twice (lower and upper)
continue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great addition. Nitpick: this comment would make more sense above the whole code block (line 511)


# Print 5-6 levels (1dk deadkeys)
if deadkeys and (main_deadkey := deadkeys.get("**")):
for level_num, char in enumerate(chars[:2], start=5):
if dead_char := main_deadkey.get(char):
dead_char = main_deadkey.get(char)
if level_num == 6:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We’ve recently added a layer enumerator that you could use here:

if level_num == Layer.ODK_SHIFT:

location.set(
"class", location.get("class") + " deadKey diacritic"
)
print_char(location, char)

# Print 5-6 levels (1dk deadkeys)
if deadkeys and (main_deadkey := deadkeys.get("**")):
for level_num, char in enumerate(chars[:2], start=5):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I’ve recently added a layer enumerator but I’ve totally missed this line. You could apply it here:

for level_num, char in enumerate(chars[:2], start=Layer.ODK):

@fabi1cazenave fabi1cazenave merged commit d3ab990 into OneDeadKey:main Feb 4, 2024
12 checks passed
fabi1cazenave pushed a commit that referenced this pull request Feb 5, 2024
fabi1cazenave pushed a commit that referenced this pull request Feb 5, 2024
fabi1cazenave pushed a commit that referenced this pull request Feb 5, 2024
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.

2 participants