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 semver color output representation #73

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

noniagriconomie
Copy link
Contributor

@noniagriconomie noniagriconomie commented Aug 13, 2021

Hi

First thanks for this plugin :)

I would like to propose this kind of improvement on the output for kind of eye alerting:


Capture d’écran 2021-09-06 à 11 22 01


Thank you!

@pyrech
Copy link
Owner

pyrech commented Aug 13, 2021

Hi!

Thank you for your contribution. It really like it 👍

Could you share some screenshots of the colored rendering? I'm afraid the uppercase word looks a bit "rough" - may be we could add a dash or some sort of punctuation. What do you think?

FYI I'm going on vacation for a week so I may take some time to answer 😉

@noniagriconomie
Copy link
Contributor Author

noniagriconomie commented Aug 13, 2021

@pyrech PR code and desc updated, feel free to comment/ give feedback on both outputs

Yes I am too until eomonth, have a nice break :)

@noniagriconomie noniagriconomie changed the title Add semver representation as output Add semver color output representation Aug 13, 2021
@noniagriconomie
Copy link
Contributor Author

friendly ping @pyrech 👋

@pyrech
Copy link
Owner

pyrech commented Sep 6, 2021

Thanks for the ping 😅. I'll review this PR right now 👍

Copy link
Owner

@pyrech pyrech left a comment

Choose a reason for hiding this comment

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

Good job, I like the rendering 👍
I just left 2 minor comments

src/OperationHandler/UpdateHandler.php Outdated Show resolved Hide resolved
src/OperationHandler/UpdateHandler.php Outdated Show resolved Hide resolved
@noniagriconomie
Copy link
Contributor Author

@pyrech

to better handle (uncommon) cases of downgrade, shouldn't we compare if major version are just different instead of one greater than the other?

yes we can, I'll update the comparison operation

appart from that, do you prefer the first or second display (PR desc, for my part I prefer the first)

thx !

@pyrech
Copy link
Owner

pyrech commented Sep 6, 2021

I also prefer the first 👍

@noniagriconomie
Copy link
Contributor Author

@pyrech here you are, PR code and desc updated accordingly :)

@pyrech
Copy link
Owner

pyrech commented Sep 6, 2021

Oops, I needed to approve the run of CI and it looks like the tests are broken (I think you just need to fix assertion with the added precisions)

@noniagriconomie
Copy link
Contributor Author

Indeed, while checking the test, I found that I had to deal with dev-master naming of version, thus I added a check to only handle version with . inside
thx again :)

@pyrech
Copy link
Owner

pyrech commented Sep 6, 2021

Awesome, lets' merge and release this one! Thanks @noniagriconomie for your contribution!

@pyrech pyrech merged commit 822edf5 into pyrech:main Sep 6, 2021
@noniagriconomie noniagriconomie deleted the ft-semver branch September 6, 2021 12:40
@mikemand
Copy link
Contributor

mikemand commented Oct 6, 2021

Is there anything that can be done for other terminal themes? The white on yellow is not legible.

Screen Shot 2021-10-06 at 2 43 38 PM

Edit: I'm using Hyper terminal with Material theme

@pyrech
Copy link
Owner

pyrech commented Oct 12, 2021

Hello @mikemand

This looks indeed not optimal. Maybe we should remove these colored background to fix these accessibility problems. Would you mind to give a try ?

@mikemand
Copy link
Contributor

mikemand commented Oct 12, 2021

Hi @pyrech

Sure! I'll get a PR opened ASAP. Should we keep the colors, and just use <fg> instead of <bg>? I'm afraid the yellow will still be bad on a white background too, so maybe a different color for that as well?

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.

3 participants