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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add class="" wrapper to attribute values for CSS styling #4747

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gogowitsch
Copy link
Contributor

This PR addresses or contains one new feature:

  • Wrap HTML output of property values in CSS-able <span /> tag.

    This will allow for custom styling such as an icon before the text
    added by wiki admins.

I couldn't get the PHPUnit tests to run locally yet. After 2 hours, I give up for today and let Travis do the work 馃槩

Christian Bl盲ul added 2 commits April 25, 2020 20:45
This should help IDEs and static code analyzers
This will allow for custom styling such as an icon before the text
added by wiki admins.
@mwjames
Copy link
Contributor

mwjames commented Apr 25, 2020

I couldn't get the PHPUnit tests to run locally yet.

What was/is the problem? The documentation [0] should be fairly clear on what to do, or not?

[0] https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/tests/README.md

@gogowitsch
Copy link
Contributor Author

Thanks for the quick reaction! I figured it out from the Travis output: the PHPUnit version cannot be 4.8, it must be 6.5. Now all is fine locally. I already committed an updated README.md, but you beat me 馃槉

@gogowitsch
Copy link
Contributor Author

gogowitsch commented Apr 25, 2020

Assuming I get the PHPUnit test coverage green, do you think adding a <span /> to the HTML output is a viable thing to do in this code base?

@mwjames
Copy link
Contributor

mwjames commented Apr 25, 2020

do you think adding a to the HTML output is a viable thing to do in this code base?

For now I cannot see an issue to add a span for the HTML output but of course it will generate the span for each text value that comes in contact with a DataValue instance when a HTML output is requested.

@mwjames
Copy link
Contributor

mwjames commented Apr 25, 2020

For now I cannot see an issue to add a span for the HTML output but of course it will generate the span

@kghbln Any insights her? I want to avoid a similar issue as with the plainlist vs. list output discussion during the 3.0 release.

@jaideraf
Copy link
Member

jaideraf commented Apr 25, 2020

Won't this change affect plainlist format?

If the output should include class attributes to HTML elements then result format "List" needs to be used. [0]

[0] https://www.semantic-mediawiki.org/wiki/Help:Plainlist_format

EDIT: @mwjames was fast again 馃

@mwjames
Copy link
Contributor

mwjames commented Apr 25, 2020

Won't this change affect plainlist format?

Most likely for text (_txt) values, yes.

@kghbln
Copy link
Member

kghbln commented Apr 26, 2020

Any insights her? I want to avoid a similar issue as with the plainlist vs. list output discussion during the 3.0 release.

I believe this is about in-text annotations not added via templates. I personally would use a template even for this too, i.e. embedding single values, and not change the output behavior of the value itself.

Also by default things should stay as they are, meaning if a wiki wants to have spans for these values there should be a dedicated config parameter or a feature flag to an existing parameter to enable rich formatting.

The issue with the list format rework was that it changed the default behavior which was in retrospect not the best decision (list/plainlist <--> list/richlist). We should avoid something like this here.

@mwjames
Copy link
Contributor

mwjames commented May 2, 2020

This will allow for custom styling such as an icon before the text
added by wiki admins.

@fonata While I think I grasp the idea here, maybe explaining the exact intend or use case would help others on this thread to understand what you are trying to achieve to see whether a better solution is available otherwise we end up with something that causes unnecessary opposition.

@akuckartz
Copy link

@fonata Ping

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

5 participants