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

Add poet-theme to Melpa #5373

Merged
merged 1 commit into from Mar 25, 2018
Merged

Add poet-theme to Melpa #5373

merged 1 commit into from Mar 25, 2018

Conversation

kunalb
Copy link
Contributor

@kunalb kunalb commented Mar 17, 2018

Adds a new theme that tries to make writing prose more elegant: https://github.com/kunalb/poet

Tested by running a sandbox and installing the theme locally based on the documentation.

Brief summary of what the package does

It's a new theme that styles variable-pitch mode carefully; eg. normal text is in georgia while code will still be shown in a monospace font.

Direct link to the package repository

https://github.com/kunalb/poet

Your association with the package

I'm the creator/maintainer.

Relevant communications with the upstream package maintainer

None Needed

Checklist

Please confirm with x:

Adds a new theme that tries to make writing prose more elegant: https://github.com/kunalb/poet

Tested by running a sandbox and installing the theme.
@purcell
Copy link
Member

purcell commented Mar 18, 2018

Thanks for this. A few points of feedback:

  • This is a theme, yet it defines a poet-mode minor mode, which isn't appropriate. If this is really a theme, users will be able to simply load it with load-theme or M-x customize-themes, so if that isn't the whole story then it will confuse them. Also the theme is globla, while the minor mode is buffer-local, which is weird. You can instead include line-spacing as a custom variable set by the theme, and my advice would be to drop the minor mode and its code to toggle flyspell, linum, typo etc.
  • The provide form doesn't match the name of the file. package-lint should have warned you about this.
  • I know the point is to be opinionated, but this theme simply won't work on many users' machines, given the hard-coded font sizes and families - both of which themes should avoid. I'd suggest that in its current form it may not be useful for a wider audience.

@kunalb
Copy link
Contributor Author

kunalb commented Mar 18, 2018

Thanks! I'll update the theme.

  • Line spacing was the biggest thing I wanted, I think olivetti-mode is much better than the other things; I'll get rid of the minor mode. Would you have a pointer to setting line-spacing as a custom variable?
  • Hmm, I thought I'd addressed all lint, I'll check again.
  • Fonts I was definitely going to make customizable, I hadn't thought about the font sizes – thanks for catching that! I'll replace them with 1.x - 2.0 etc.

@purcell
Copy link
Member

purcell commented Mar 18, 2018

Would you have a pointer to setting line-spacing as a custom variable?

Yeah, check out custom-theme-set-variables, which you can use similarly to custom-theme-set-faces, but to associate variable values with a theme.

Fonts I was definitely going to make customizable, I hadn't thought about the font sizes – thanks for catching that! I'll replace them with 1.x - 2.0 etc.

Nice. Re. making the fonts customizable, that's kinda what the customization and face mechanism is for in the first place: you might consider inheriting faces from fixed-pitch, variable-pitch etc, but not actually customizing those base faces in this theme. That way, the theme can build on whatever fonts the users have selected for the base faces.

@kunalb
Copy link
Contributor Author

kunalb commented Mar 18, 2018

Just updated the theme: take another look! I think I've caught most of the feedback, moved poet-mode's behaviour (with all the modes) to documentation, set height as a custom-variable, made heights relative.

I do pick up and cache the default font-face on theme load so that variable pitch can't go and nuke it otherwise, I'm not sure if there are better options there because I do want it to be usable without variable pitch mode too.

@purcell purcell merged commit 40bbe29 into melpa:master Mar 25, 2018
@purcell
Copy link
Member

purcell commented Mar 25, 2018

Cool, merged now - welcome the MELPA! 🎉

@kunalb
Copy link
Contributor Author

kunalb commented Mar 25, 2018

Yay, Thanks!

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.

None yet

2 participants