Skip to content

Conversation

nojhan
Copy link
Collaborator

@nojhan nojhan commented Dec 29, 2024

Adds a theme that displays the name of each of the prompt sections, along with a clickable hyperlink to the corresponding documentation.

@nojhan nojhan added the theme Related to the display of data and/or generation of the prompt label Dec 29, 2024
@nojhan nojhan self-assigned this Dec 29, 2024
Adds a theme that displays the name of each of the prompt sections,
along with a clickable hyperlink to the corresponding documentation.
Copy link
Collaborator

@Rycieos Rycieos left a comment

Choose a reason for hiding this comment

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

I apologize for the delay. I hope to be more present now.

This is looking great. My biggest concern is the screenshots not matching the other themes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The screenshots of the other themes use the tools/theme-preview.sh script, because it mocks the data functions to have generic and consistent data. Please update these images to use that as well.

If you used your own environment because that script does not demo the features as well as you want, then I think we need to update the mocked data in the script.

*************

The included ``themes/explain/explain.theme`` file includes a theme that
add a label to each section of the default theme, along with a clickable link
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
add a label to each section of the default theme, along with a clickable link
adds a label to each section of the default theme, along with a clickable link

Liquid Prompt configuration
---------------------------

The LP_ENABLE_HYPERLINKS configuration variable should be set to 1 for the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs to be linked

Suggested change
The LP_ENABLE_HYPERLINKS configuration variable should be set to 1 for the
The :attr:`LP_ENABLE_HYPERLINKS` configuration variable should be set to 1 for the

Comment on lines +89 to +90
points to the variable ``LP_BRACKET_OPEN``, as documented on the following page:
https://liquidprompt.readthedocs.io/en/stable/theme/default.html
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for a manual link.

Suggested change
points to the variable ``LP_BRACKET_OPEN``, as documented on the following page:
https://liquidprompt.readthedocs.io/en/stable/theme/default.html
points to the variable :attr:`LP_BRACKET_OPEN`.

points to the variable ``LP_BRACKET_OPEN``, as documented on the following page:
https://liquidprompt.readthedocs.io/en/stable/theme/default.html

Unless explicitly set, *explain* will set LP_MARK_PREFIX as a single newline.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Unless explicitly set, *explain* will set LP_MARK_PREFIX as a single newline.
Unless explicitly set, *explain* will set :attr:`LP_MARK_PREFIX` as a single newline.

Comment on lines +98 to +106
- ``LP_MARK_DEV_OPEN``
- ``LP_MARK_DEV_CLOSE``
- ``LP_MARK_DEV_MID``
- ``LP_MARK_MODULES_OPEN``
- ``LP_MARK_MODULES_SEP``
- ``LP_MARK_MODULES_CLOSE``
- ``LP_MARK_ENV_VARS_OPEN``
- ``LP_MARK_ENV_VARS_SEP``
- ``LP_MARK_ENV_VARS_CLOSE``
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might as well let these be linked:

Suggested change
- ``LP_MARK_DEV_OPEN``
- ``LP_MARK_DEV_CLOSE``
- ``LP_MARK_DEV_MID``
- ``LP_MARK_MODULES_OPEN``
- ``LP_MARK_MODULES_SEP``
- ``LP_MARK_MODULES_CLOSE``
- ``LP_MARK_ENV_VARS_OPEN``
- ``LP_MARK_ENV_VARS_SEP``
- ``LP_MARK_ENV_VARS_CLOSE``
- :attr:`LP_MARK_DEV_OPEN`
- :attr:`LP_MARK_DEV_CLOSE`
- :attr:`LP_MARK_DEV_MID`
- :attr:`LP_MARK_MODULES_OPEN`
- :attr:`LP_MARK_MODULES_SEP`
- :attr:`LP_MARK_MODULES_CLOSE`
- :attr:`LP_MARK_ENV_VARS_OPEN`
- :attr:`LP_MARK_ENV_VARS_SEP`
- :attr:`LP_MARK_ENV_VARS_CLOSE`

LP_EXPLAIN_MARK_BODY_TOP=━
LP_EXPLAIN_MARK_BODY_BOTTOM=━

LP_MARK_PREFIX=$'\n'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do only some of these files have this line? I don't see why they need it at all, since it is the default anyway.

Comment on lines +64 to +66
The ``themes/explain/preset-chevrons.conf`` preset requires a font supporting
"Box Drawings Diagonal" characters (approved as part of Unicode version 1.1
in 1993):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not quite true. While , , and were added in Unicode v1.1, 🮠, 🮡, 🮢, 🮣, 🮤, and 🮥 were added in Unicode v13.0, which was released in 2020. None of my devices have been able to render those (Windows 11, MacOS 14.7, Android 14).

I recommend finding some older characters that look similar. If that is not possible, update this docs section, and consider adding a link to a font that supports those characters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme Related to the display of data and/or generation of the prompt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants