-
Notifications
You must be signed in to change notification settings - Fork 164
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
Improved Tag Support: Tags in Excel and Tags for Examples Blocks in Json, Dhtml, Html, and Word formats #424
Conversation
Hi, Thank you for your time and effort. Unfortunately, the build does not pass. It looks like some of the unit tests fail. Could you please verify whether the unit tests pass on your development machine? Thanks! |
The build is still failing with two failing unit tests :-( Could you please check again? |
Great, the build passes now. In order to show off your work, could you please add some tags to the examples? This feature file would be a good spot: https://github.com/picklesdoc/pickles/blob/develop/src/Pickles/Examples/Features/03ScenarioOutline/ScenarioOutline.feature People will then be able to see the tags at http://www.picklesdoc.com/pickles/Output/Html/Features/03ScenarioOutline/ScenarioOutline.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for your contribution. This is a significant improvement to Pickles. I'm planning to release a new version soon and I want to include this feature!
I'd like you to change a couple of things - I added comments to individual sections.
I noticed that you did not include code for the Excel output format. Is there any particular reason for that?
@@ -44,7 +46,8 @@ public JsonExample Map(Example example) | |||
{ | |||
Name = example.Name, | |||
Description = example.Description, | |||
TableArgument = this.tableMapper.Map(example.TableArgument) | |||
TableArgument = this.tableMapper.Map(example.TableArgument), | |||
Tags = (example.Tags ?? new List<string>()).ToList(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide unit tests to make sure the mapping is done correctly - ExampleToJsonExampleMapperTests
is the appropriate class.
body.GenerateParagraph("Examples: " + example.Description, "Heading3"); | ||
this.wordTableFormatter.Format(body, example.TableArgument); | ||
var paragraph = new Paragraph(new ParagraphProperties(new ParagraphStyleId { Val = "Heading3" })); | ||
paragraph.Append( new Run( new RunProperties(), new Text( "Examples: " + example.Description ) ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spacing around the parenthesis does not conform to the project's style.
@@ -122,6 +123,7 @@ private XElement FormatExamples(ScenarioOutline scenarioOutline) | |||
new XAttribute("class", "examples"), | |||
new XElement(this.xmlns + "h3", "Examples: " + example.Name), | |||
this.htmlDescriptionFormatter.Format(example.Description), | |||
(example.Tags == null || example.Tags.Count == 0) ? null : new XElement(this.xmlns + "span", HtmlScenarioFormatter.CreateTagElements(example.Tags.OrderBy(t => t).ToArray(), this.xmlns)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to be consistent with the formatting of tags at other places, could you please use a <p class="tags">
- the code should be similar to https://github.com/picklesdoc/pickles/blob/develop/src/Pickles/Pickles.DocumentationBuilders.Html/HtmlScenarioFormatter.cs#L59
Better yet: refactor the various places where tags are added so they all call the same method.
if ( feature.Tags.Count != 0 ) | ||
{ | ||
var paragraph = new Paragraph( new ParagraphProperties( new ParagraphStyleId {Val = "Normal"} ) ); | ||
var tagrunProp = new RunProperties( new Italic(), new Color {ThemeColor = ThemeColorValues.Text2} ) {Bold = new Bold() {Val = false}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the visual style of this version.
body.GenerateParagraph("Examples: " + example.Description, "Heading3"); | ||
this.wordTableFormatter.Format(body, example.TableArgument); | ||
var paragraph = new Paragraph(new ParagraphProperties(new ParagraphStyleId { Val = "Heading3" })); | ||
paragraph.Append( new Run( new RunProperties(), new Text( "Examples: " + example.Description ) ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the visual style of the tags of a feature better. Could you adapt this code so it uses the same visual style as WordFeatureFormatter?
No there is no technical raison to not include code for excel.
In fact, it seems that for excel tags are not exported, even for feature and scenario. Therefore, I have not do it.
Anyway, I may do it if needed. Please, just Ask.
De : Dirk Rombauts [mailto:[email protected]]
Envoyé : mardi 7 février 2017 12:59
À : picklesdoc/pickles <[email protected]>
Cc : Leveille, Pascal (Nokia - FR) <[email protected]>; Author <[email protected]>
Objet : Re: [picklesdoc/pickles] Add tag output support for Examples: (for json, dhtml, html, word) (#424)
@dirkrombauts requested changes on this pull request.
|
I would be grateful if you could do tag support for Excel! |
… space style, more test, use tags html class ...)
…e settings into () and {}
Great work on the tag support in Excel! Thanks. I still have two remarks about the other formats:
|
Yes, I will do it tomorrow.
I still have two remarks about the other formats:
1. The word format: the tags of example blocks should be on separate lines, like the tags of scenarios. If they are not on separate lines, they will show up in the table of contents. See the attached screenshot of the word output.
2. The dhtml format: this format has the tags on a line before the scenario title or feature title. In order to be consistent, could you please move the tags of example blocks to the line before Example (see the screenshot of the dhtml output).
|
…put Format (excel, word, dhtml, html)
Many thanks @pleveill! |
Released in version 2.13.0. |
I tweeted an extra thanks to you for going above and beyond the call of duty in improving tag support across all formats! See https://twitter.com/picklesdoc/status/830004886146281472 |
No description provided.