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

feat(ui): Make file types themeable #245

Merged
merged 9 commits into from
Sep 16, 2023

Conversation

cfxegbert
Copy link
Contributor

@cfxegbert cfxegbert commented Sep 11, 2023

Addresses issue #235.

@cfxegbert cfxegbert requested a review from cafkafk as a code owner September 11, 2023 05:36
@cafkafk cafkafk linked an issue Sep 11, 2023 that may be closed by this pull request
@cfxegbert
Copy link
Contributor Author

Just noticed https://github.com/eza-community/eza/pull/63/files. Should I refactor to be more like #63? Should I rename immediate to build?

@cafkafk
Copy link
Member

cafkafk commented Sep 11, 2023

Should I refactor to be more like #63?

If you wanted to, perhaps you could do so in a follow up PR (which would make it easier to see the diff). I don't think you have too, but maybe I'm missing something.

Should I rename immediate to build?

Build is probably more intuitive, immediate might be more "correct". I may lean slightly to being more intuitive, but I think you should make the pick you prefer tbh.

@PThorpe92
Copy link
Member

If you wanted to, perhaps you could do so in a follow up PR (which would make it easier to see the diff).

I have been trying to diff your PR and the old one by switching tabs. and I would heavily agree with this.

I would say that if there is anything you feel 63 does better / cleaner than yours, implement it and they can be essentially merged into one, and we can close 63 in favor of this one 👍

cafkafk
cafkafk previously approved these changes Sep 14, 2023
Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

LGTM 👍, haven't tested this but have tested that it doesn't seem to break anything.

Sidequest: While you have these parts of the code fresh in your mind, would you like to document them? If not just ping me and I'll merge.

src/theme/ui_styles.rs Show resolved Hide resolved
src/theme/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

Still works for me, and integration tests work, so merging, ty for the PR

@cafkafk cafkafk merged commit 3f09e4b into eza-community:main Sep 16, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

feat: Custom colors for file categories
3 participants