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 HTML formatter #258

Merged
merged 13 commits into from
Mar 8, 2022
Merged

add HTML formatter #258

merged 13 commits into from
Mar 8, 2022

Conversation

Gummibeer
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
Fixed tickets #232

This PR adds the ability to format the output as HTML and redirect it to a file, if wanted.

The result looks like this:
screencapture-localhost-63342-phpinsights-dashboard-html-2019-08-28-14_58_11

Under the hood Twig is used for templating. If wanted I can change this to be only a composer suggestion and have the user to install it by his own if he wants to use HTML formatter?

@nunomaduro
Copy link
Owner

So much potential in this pull request, we really need twig here?

@nunomaduro nunomaduro added the enhancement New feature or request label Aug 28, 2019
@Gummibeer
Copy link
Contributor Author

It makes it a lot easier than doing '<strong>'.$metric.'</strong>'. Without view files it would get a lot more code in the formatter and not this readable.
I understand your problem with it! And can refactor it out, but with it's easier and more readable.

@Gummibeer
Copy link
Contributor Author

Another idea would be to use mustache, handlebars or any other JS template engine that could be loaded via CDN to get around the composer dependency but still don't render with string concatenation.

@olivernybroe
Copy link
Collaborator

I get why you choose to use twig, however it does add more dependencies. I think I might be fine with adding twig as it is much more needed when we add more advanced styling.

Another suggestion would be to use the Json formatter and then parse the Json in JavaScript and handle it that way?

@Gummibeer
Copy link
Contributor Author

@olivernybroe that was the idea in my last comment. It would only require a single HTML stub/template and no additional dependencies.
Even if most JS template engines I know lack real conditions, anything like twig filters and so on. Which would increase the view data preparation in PHP.
And using Vue is a bit overloaded I think!? For me the resulted HTML file should work out of the box and via simple file:// link which is a problem for a lot of JS engines.
How about the optional twig dependency, let the user require it and in best case a small rework to support twig 1 & 2.
This won't install twig for everyone and everyone should find an installable version of twig if both major versions are supported!?

@olivernybroe
Copy link
Collaborator

We already have a lot of other dependencies that limit the user to the newest version and we have a workaround for dependency problems in the docs.

I think I'll let @nunomaduro decide this. For me, I'll be okay with it.
I am not a fan of the idea that you have to composer install twig to use the HTML formatter, then I'll prefer the whole formatter to be a separate package that you just install in and then that package can have twig dependency.

@Gummibeer
Copy link
Contributor Author

Anything I have to do here?

@olivernybroe
Copy link
Collaborator

@Gummibeer Sorry for the wait.

I think you should just add Twig in the suggest section of composer with a comment saying that it is needed for the html formatter. Then in the docs also add that you need to run composer require....

With those changes and the travis passing I think it's ready for the merging and then we can always change it later on.

@Gummibeer
Copy link
Contributor Author

Ok, will do so.

@Gummibeer
Copy link
Contributor Author

@olivernybroe should be fine now.

@olivernybroe
Copy link
Collaborator

@Gummibeer Looks great. Just need the test to pass then.

You can run composer test locally, so you don't have to wait for travis-ci.

One of the errors, you need to silence in the phpstan file, just like we did with the json formatter.

- '#NunoMaduro\\PhpInsights\\Application\\Console\\Formatters\\Json has an unused parameter \$input#'

The other ones are stuff you need to program around 😃

@Gummibeer
Copy link
Contributor Author

@olivernybroe just as an idea/hint: atm its' nearly impossible to find the thing that has to be fixed because of tons of errors (in phpinsights) that are accepted. And it's also not a great feeling that I'm responsible to fix this single issue that made the analyser switch from green to red if there is a list of hundreds of issues that's ok to be there.

@olivernybroe
Copy link
Collaborator

@Gummibeer Hmm, I get you, do you have any solution for it?

@Gummibeer
Copy link
Contributor Author

Fix them 🤪😅
Or ignore them like phpstan. But selective ignore wasn't possible for phpinsights?

@olivernybroe
Copy link
Collaborator

Some of them should prob. be ignored, some should be fixed :)

We have the feature to silence a specific error per file, but we need features for silence based on message and per folder.

You are more than welcome to start fixing them! ♥️

Copy link
Collaborator

@olivernybroe olivernybroe left a comment

Choose a reason for hiding this comment

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

I think this is ready for merging.

@nunomaduro you wanna check it out first?

@nunomaduro
Copy link
Owner

nunomaduro commented Sep 24, 2019

I don't think so. I think he have done a great job. But on the design we do way better.

@Gummibeer
Copy link
Contributor Author

@nunomaduro do you have any example/layout? I can also use CDN tailwind. But I would like to don't include any SASS/JS in the package itself just for a formatter output style!?
And isn't this formatter meant for a fast and easy to read output? Everyone who want's to show this with his company branding or whatever can use the JSON formatter and display it.

@nunomaduro
Copy link
Owner

I think we will need to work with a designer on this one. Before we jump on using tailwindcss. Do you have designer skills?

@Gummibeer
Copy link
Contributor Author

Nope, I even wouldn't say that I have frontend skills. :D

…to ft-html-formatter

# Conflicts:
#	docs/get-started.md
#	phpstan.neon.dist
#	src/Application/Console/Definitions/AnalyseDefinition.php
#	src/Application/Console/Formatters/FormatResolver.php
@nunomaduro
Copy link
Owner

Can you make a tweet asking for a help on the designer in this issue? If you don't want, I can do it.

@Gummibeer
Copy link
Contributor Author

@dmitryuk
Copy link

Any updates about the issue?

@olivernybroe
Copy link
Collaborator

@dmitryuk Still stuck on the need for a beautiful design for it unfortunately.

@dmitryuk
Copy link

@olivernybroe I will try to help us with a designer, but could you shortly describe what do you want to change in the current implementation?

@olivernybroe
Copy link
Collaborator

@dmitryuk I suck at designing, so not the best at coming with suggestions for this :)
Maybe @nunomaduro has some ideas?

@emjayess
Copy link

emjayess commented Mar 3, 2021

Cc: @feline-blue > heya Lauren this potential sweet feature needs a little injection of vanilla (no css kit dependency) css design! Whaddaya think?

@Gummibeer
Copy link
Contributor Author

Since I've done a lot more TailwindCSS in the past and also some more websites from scratch I could give it another go. But can't provide a design upfront.
Otherwise @caneco or @lorisleiva possibly have an idea/time for something pretty? 🤔

JustSteveKing added a commit that referenced this pull request Mar 8, 2022
Handling PR #258 to import changes and fix conflicts
@JustSteveKing JustSteveKing merged commit 6bc4bb9 into nunomaduro:master Mar 8, 2022
@dmitryuk
Copy link

dmitryuk commented Mar 8, 2022

Good work!

@squpshift
Copy link

squpshift commented Feb 21, 2023

Hey, this appears to have been reverted in 5e84dab. Why? :(

@gemal
Copy link

gemal commented Mar 22, 2023

Any comment on why the HTML format was removed? Was there a problem?

@ArielMejiaDev
Copy link

Could anyone please tell me why this option has been removed? dependencies?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants