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

[WIP, UI] Force Ace font #4090

Closed
wants to merge 2 commits into from
Closed

[WIP, UI] Force Ace font #4090

wants to merge 2 commits into from

Conversation

Felienne
Copy link
Member

@Felienne Felienne commented Mar 7, 2023

This might be a fix for #3823?

Can you take a peek @alaaeddin-swidan?

It does make the editor look ugly in Latin because the font is too small, but if this addresses the issue I am sure we can patch that too. If this does not work, in any case it can be a branch for us to try stuff out in (other options mentioned in #3823) because you now know where to fiddle with the css?

@Felienne Felienne changed the title [WIP, UI] Force font [WIP, UI] Force Ace font Mar 7, 2023
@ghost
Copy link

ghost commented Mar 7, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@vzsky
Copy link
Contributor

vzsky commented Mar 7, 2023

Does not work for Thai. I did change a few css but still no luck. I think it might be more or less related to ajaxorg/ace#3141 and this open issue ajaxorg/ace#460

@Felienne
Copy link
Member Author

Felienne commented Mar 7, 2023

Does not work for Thai. I did change a few css but still no luck.

:(

I think it might be more or less related to ajaxorg/ace#3141 and this open issue ajaxorg/ace#460

Yes, I think that too! I took this idea from [ajaxorg/ace#460](ajaxorg/ace#460

@reedspool
Copy link
Collaborator

reedspool commented Apr 9, 2023

In one of these threads I saw the suggestion that the default monospace font in chrome "Monako" is insufficient, but that Courier works better. So something to try is

.ace_editor * {
    font-family: Courier, monospace !important;
    font-size: 12pt;
}

I tried this myself and found it worked a bit better, but didn't fix the issue completely on long lines of text. The issue seems to be worse on longer lines.

There's a chance that different fonts work best for different languages. But I fear that, at best, this kind of solution will only fix Ace half-way, leaving more issues to be patched individually as they pop up. I hope my fears are wrong though!!

@Felienne
Copy link
Member Author

Felienne commented Apr 10, 2023

In one of these threads I saw the suggestion that the default monospace font in chrome "Monako" is insufficient, but that Courier works better. So something to try is

.ace_editor * {
    font-family: Courier, monospace !important;
    font-size: 12pt;
}

I tried this myself and found it worked a bit better

Thanks for diving in deeper! Somehow, I cannot seem to reproduce the issue as shown in the video of @alaaeddin-swidan above, both in Chrome for macOS and Safari backspace does go the right way in Arabic (towards the gutter) and I am also able to select a word and delete the whole word with backspace too. How have you reproduced the issue?

, but didn't fix the issue completely on long lines of text. The issue seems to be worse on longer lines.

Should that be the case we can maybe patch that a bit by giving a warning for long lines.

There's a chance that different fonts work best for different languages.

If this works, we can set this for for Arabic only, that is probably not all too hard with a mix of a bit of js and css.

But I fear that, at best, this kind of solution will only fix Ace half-way, leaving more issues to be patched individually as they pop up.

This is probably true but giving we are not likely to move over to CodeMirror soon, half fixed is better than nothing?

I hope my fears are wrong though!!

I have pushed your suggestion, maybe @alaaeddin-swidan can take a look, or @vzsky for Thai (assuming this has the same root cause)?

@Felienne
Copy link
Member Author

we are going to slowly move away from Ace, so this can be closed

@Felienne Felienne closed this Jun 14, 2023
@Felienne Felienne deleted the ace-cursor-issue branch December 9, 2023 16:06
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