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

Implement new pie and pie-light styles #1238

Merged
merged 7 commits into from Dec 19, 2021
Merged

Conversation

isidentical
Copy link
Contributor

Resolves #1237.

Demos:
asciicast
asciicast

@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2021

Codecov Report

Merging #1238 (6afcf21) into master (4d7d6b6) will decrease coverage by 0.88%.
The diff coverage is 93.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1238      +/-   ##
==========================================
- Coverage   97.28%   96.40%   -0.89%     
==========================================
  Files          67       82      +15     
  Lines        4235     5560    +1325     
==========================================
+ Hits         4120     5360    +1240     
- Misses        115      200      +85     
Impacted Files Coverage Δ
tests/test_binary.py 100.00% <ø> (ø)
httpie/compat.py 31.11% <27.90%> (-68.89%) ⬇️
httpie/output/lexers/http.py 52.00% <52.00%> (ø)
tests/conftest.py 77.14% <58.33%> (-9.82%) ⬇️
tests/test_ssl.py 91.01% <66.66%> (-3.93%) ⬇️
httpie/output/formatters/colors.py 89.13% <76.74%> (-4.45%) ⬇️
httpie/manager/__main__.py 82.35% <82.35%> (ø)
httpie/models.py 94.73% <83.33%> (-2.64%) ⬇️
httpie/manager/plugins.py 93.87% <92.66%> (ø)
httpie/manager/core.py 92.85% <92.85%> (ø)
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e30ec6b...6afcf21. Read the comment docs.

@isidentical isidentical force-pushed the unify-colors branch 3 times, most recently from 94690e3 to d1ad5ac Compare December 16, 2021 16:38
@isidentical isidentical force-pushed the unify-colors branch 2 times, most recently from 5681651 to c77621a Compare December 17, 2021 10:11
Comment on lines 296 to 308
def make_styles(name, raw_styles):
for mode, shade in [
('light', '700'),
('universal', '600'),
('dark', '500')
]:
yield type(
name.format(mode=mode.title()),
(pygments.style.Style,),
{
'styles': format_style(raw_styles, shade)
}
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably should use a more robust methodology in the future (for -100 -200 -300 etc). Something like this

for shade in range(100, 800, 100):
    name = SHADE_NAMES.get(shade, f"pie-{shade}")
    for style_type, style_map in [("Header", HEADER_COLORS), ("Body", BODY_COLORS)]:
         create_styles()

though I plan to omit this for this release, and implement this after this gets merged (if there are no objections).

Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

Copy link
Member

@jkbrzt jkbrzt left a comment

Choose a reason for hiding this comment

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

Good stuff 💅 A few remaining tweaks.

docs/README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
Comment on lines 296 to 308
def make_styles(name, raw_styles):
for mode, shade in [
('light', '700'),
('universal', '600'),
('dark', '500')
]:
yield type(
name.format(mode=mode.title()),
(pygments.style.Style,),
{
'styles': format_style(raw_styles, shade)
}
)
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

Comment on lines 273 to 308
def format_style(raw_styles, shade):
def get_shade(part):
if part not in COLOR_PALETTE:
return part

color_code = COLOR_PALETTE[part]
if isinstance(color_code, dict) and shade in color_code:
return color_code[shade]
else:
return color_code

def format_value(value):
return " ".join(
get_shade(part)
for part in value.split()
)

return {
key: format_value(value)
for key, value in raw_styles.items()
}


def make_styles(name, raw_styles):
for mode, shade in [
('light', '700'),
('universal', '600'),
('dark', '500')
]:
yield type(
name.format(mode=mode.title()),
(pygments.style.Style,),
{
'styles': format_style(raw_styles, shade)
}
)
Copy link
Member

Choose a reason for hiding this comment

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

These are tied to the Pie themes, so I’d either reflect that in their names or move them to palette.

@@ -0,0 +1,138 @@
# Copy the brand palette

COLOR_PALETTE = {
Copy link
Member

Choose a reason for hiding this comment

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

PIE_COLOR_PALETTE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need this prefix? I assumed these were going to be the default colors we use after this PR (e.g on progress bars, on regular colored outputs etc.)

Copy link
Member

Choose a reason for hiding this comment

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

Good question. I thought we’d always re-use the effective --style even for the UI, because if I use the terminal scheme, for example, then I’d like to see those colors everywhere. In that case, giving it the PIE_ prefix would make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I especially put this into a separate package, httpie.output.ui, since I plan to (at least feel like) this is going to be the place where we would manage our rich UI (progress bars, themes, highlighting, console, etc.), and those probably will use this theme. If you feel strong about it, I can add the PIE_ prefix though but personally I'd find this very compelling (since the fully qualified name of this is httpie.output.ui.COLOR_PLAETTE, so it is not something arbitrary).

Copy link
Member

Choose a reason for hiding this comment

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

OK. Although we should discuss this:

I thought we’d always re-use the effective --style even for the UI, because if I use the terminal scheme, for example, then I’d like to see those colors everywhere.

Comment on lines 30 to 34
PIE_STYLES = {
'pie',
'pie-light',
'pie-dark'
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like an enum/constants candidate (we refer to it from multiple places). Maybe it could also go to palette.

@isidentical isidentical force-pushed the unify-colors branch 3 times, most recently from 40e853c to cd5de1a Compare December 17, 2021 16:47
Co-authored-by: Jakub Roztocil <jakub@roztocil.co>
@jkbrzt jkbrzt merged commit 8dc6c0d into httpie:master Dec 19, 2021
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.

Create a style that matches with the HTTPie for Web/Desktop
3 participants