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.

Sign up for GitHub

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

Jump to bottom

Improve rendering of XDXF #570

Closed
wants to merge 2 commits into from
Closed

Improve rendering of XDXF #570

wants to merge 2 commits into from

Conversation

soshial
Copy link
Contributor

@soshial soshial commented Jun 28, 2024 •
edited
Loading

Completed:

  • use CSS for all XDXF element (decrease size of output dict file)
  • created a list (ol / ul) for nested <def>s
  • support loading of <abbreviations> data
  • show js tooltips for abbreviations
  • fixed showing hidden <gr>
  • fixed losing spaces before XDXF tags
  • add braces to <co>
  • show CSS instead of inlined HTML tags for: <ex>, <pos>, <abbr>, <k>, <gr>, <mrkd>

Screenshot:
Screenshot 2024-06-28 at 21 27 14

ilius
ilius reviewed Jun 29, 2024
View reviewed changes
pyglossary/plugins/xdxf/__init__.py Outdated
@@ -125,6 +123,9 @@ class Reader:

_html: bool = True
_xsl: bool = False
_has_added_css: bool = False
Copy link
Owner

Choose a reason for hiding this comment

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

These class attrs are used for read/write options! Don't use them to keep an "instance state" that you then change internally.

Copy link
Owner

Choose a reason for hiding this comment

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

Switch to instance attrs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, fixed ruff errors

@ilius
Copy link
Owner

ilius commented Jun 29, 2024

Fix ruff errors.
Make sure it work as good as before without CSS (not all formats/applications support CSS)
And why are tests failing?

@soshial soshial force-pushed the xdxf-add-js-tooltips-and-css branch 2 times, most recently from 2ee9f62 to 5a4ec00 Compare July 6, 2024 14:27
@ilius
Copy link
Owner

ilius commented Jul 18, 2024

Add an option to enable css.
Not all dictionaries / formats support css file.
Then the tests should pass.

@soshial
Copy link
Contributor Author

soshial commented Jul 30, 2024 •
edited
Loading

Add an option to enable css.
Not all dictionaries / formats support css file.

Do you mean, that CSS rules should be part of <head> of each gloss? Or not using CSS at all as it was before?

@ilius
Copy link
Owner

ilius commented Aug 1, 2024 •
edited
Loading

Add an option to enable css.
Not all dictionaries / formats support css file.

Do you mean, that CSS rules should be part of <head> of each gloss? Or not using CSS at all as it was before?

On second thought, let's create a new plugin called xdxf_css.
And leave xdxf plugin as is for now.

@soshial
Copy link
Contributor Author

soshial commented Aug 1, 2024

I have no idea how to add a new plugin to pyglossary. I can revert transformer.py (containing XdxfTransformer) and create transformer_css.py. But I'd need some help with wiring this stuff into pyglossary. Could you help me with this, please?

@ilius
Copy link
Owner

ilius commented Aug 1, 2024

Just add a new module/package in plugins based on the existing (like xdxf).
Then run ./scripts/gen.sh.
That's it.

@soshial
improve rendering of XDXF
10552f0 
* created a list (ol / ul) for nested <def>s
* show js tooltips for abbreviations
* fix showing hidden <gr>
* add braces to <co>
* show CSS instead of inlined HTML tags for: <ex>, <pos>, <abbr>, <k>, <gr>, <mrkd>
@soshial soshial force-pushed the xdxf-add-js-tooltips-and-css branch from 5a4ec00 to 10552f0 Compare August 1, 2024 07:59
@soshial
Copy link
Contributor Author

soshial commented Aug 1, 2024

I pushed the proposed changes, but it would mean I have to have 2 different xdxf/transform.py files. Isn't too much duplication?

ilius added a commit that referenced this pull request Sep 2, 2024
@ilius
add new plugin xdxf_css (XdxfCss) based on PR #570 by @soshial
aa6765b 
- use CSS for all XDXF element (decrease size of output dict file)
- created a list (ol / ul) for nested `<def>`s
- support loading of `<abbreviations>` data
- show js tooltips for abbreviations
- fixed showing hidden `<gr>`
- fixed losing spaces before XDXF tags
- add braces to `<co>`
- show CSS instead of inlined HTML tags for: `<ex>, <pos>, <abbr>, <k>, <gr>, <mrkd>`
@ilius
Copy link
Owner

ilius commented Sep 2, 2024

I merged this into master with these fixes:

  • Fix relative paths to current dir for .css and .js
  • Fix plugin metadata (name, description, etc)
  • Move out changes to transformer (to not break XDXF plugin)

@ilius ilius closed this Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilius ilius ilius left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@soshial @ilius