Skip to content

Feature support markdown in report description#37

Merged
kbeker merged 1 commit intomasterfrom
feature-support-markdown-in-report-description
Apr 1, 2019
Merged

Feature support markdown in report description#37
kbeker merged 1 commit intomasterfrom
feature-support-markdown-in-report-description

Conversation

@Szymiks
Copy link
Contributor

@Szymiks Szymiks commented Feb 19, 2019 •
edited by maciejSamerdak
Loading

Resolves #36

Related #106

@Szymiks Szymiks added the feature New feature label Feb 19, 2019
@Szymiks Szymiks self-assigned this Feb 19, 2019
@Karrp Karrp changed the base branch from master to feature-complex-description-input February 19, 2019 10:42
@Szymiks Szymiks force-pushed the feature-support-markdown-in-report-description branch from 581237a to c2154cf Compare February 21, 2019 09:45
maciejSamerdak
maciejSamerdak reviewed Feb 21, 2019
View reviewed changes
Copy link
Collaborator

@maciejSamerdak maciejSamerdak left a comment

Choose a reason for hiding this comment

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

Personally, I think everything is fine. There's few minor changes worth applying. I would also consult this feature, the way it's implemented, with others before merging. I don't know how markdown() function works, what result it returns. If it's raw text, perhaps we should store it in model? Then again, if there were a lot of extra characters added after conversion, it might interfere with character limit.

So I can't tell which option is more appropriate.

@maciejSamerdak maciejSamerdak force-pushed the feature-complex-description-input branch from 78183f6 to 9d0c9b3 Compare February 25, 2019 15:09
kbeker
kbeker reviewed Feb 26, 2019
View reviewed changes
Copy link
Contributor

@kbeker kbeker left a comment

Choose a reason for hiding this comment

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

Please change branch where it should be merged because now this PR is not reviewable :P

@maciejSamerdak maciejSamerdak force-pushed the feature-complex-description-input branch from 9d0c9b3 to 8db2993 Compare February 26, 2019 13:34
@Szymiks Szymiks force-pushed the feature-support-markdown-in-report-description branch from cca4e24 to f11853f Compare February 27, 2019 11:18
rwrzesien
rwrzesien reviewed Feb 28, 2019
View reviewed changes
employees/models.py
default=True,
)

def markdown_description(self):
Copy link
Contributor

@rwrzesien rwrzesien Feb 28, 2019

Choose a reason for hiding this comment

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

Please make it a property.

rwrzesien
rwrzesien reviewed Feb 28, 2019
View reviewed changes
employees/templates/employees/report_list.html
</td>
<td>
{{ report.description }}
{{ report.markdown_description|safe|linebreaksbr }}
Copy link
Contributor

@rwrzesien rwrzesien Feb 28, 2019

Choose a reason for hiding this comment

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

Could you describe the general purpose of this improvement? Is it about displaying html tags in code as html? If so, just safe filter might do it.

Copy link
Contributor Author

@Szymiks Szymiks Feb 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for than question because now I can see better how It works. Markdown_description fill our text field with html tags so without Safe filter our text field shows all that tags. Last one filter help our field to find when someone put enter and when I tested it again now I think that linbreaks works nicer than linebreaksbr, but this is only my opinion. I am not sure if I fully answered your question so if you would have more question just let me know . :)

@rwrzesien
Copy link
Contributor

rwrzesien commented Feb 28, 2019

@Szymiks Same as in previous pull request, please link to issue description with Resolves https://github.com/Code-Poets/sheetstorm/pull/21 in first pull request comment. Also it would be good, if the issue has some actual description ;)

@Szymiks Szymiks requested a review from rwrzesien February 28, 2019 10:34
@maciejSamerdak maciejSamerdak force-pushed the feature-complex-description-input branch 2 times, most recently from f6fa52a to da2a61a Compare March 22, 2019 14:33
@maciejSamerdak maciejSamerdak force-pushed the feature-complex-description-input branch 2 times, most recently from b6b32a5 to d32ec9d Compare March 28, 2019 15:41
@maciejSamerdak maciejSamerdak changed the base branch from feature-complex-description-input to master March 28, 2019 15:42
@maciejSamerdak maciejSamerdak mentioned this pull request Mar 28, 2019
Merged
@Szymiks Szymiks force-pushed the feature-support-markdown-in-report-description branch from 1c0dc0a to 2a6eb0a Compare March 29, 2019 08:53
kbeker
kbeker previously approved these changes Apr 1, 2019
View reviewed changes
Copy link
Contributor

@kbeker kbeker left a comment

Choose a reason for hiding this comment

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

If it's works for me OK

@kbeker kbeker added this to the tag 0.1.0 milestone Apr 1, 2019
maciejSamerdak
maciejSamerdak previously approved these changes Apr 1, 2019
View reviewed changes
Copy link
Collaborator

@maciejSamerdak maciejSamerdak left a comment

Choose a reason for hiding this comment

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

Yup, all good.

@kbeker kbeker dismissed stale reviews from maciejSamerdak and themself via ef4534f April 1, 2019 15:15
@kbeker kbeker force-pushed the feature-support-markdown-in-report-description branch from 2a6eb0a to ef4534f Compare April 1, 2019 15:15
kbeker
kbeker approved these changes Apr 1, 2019
View reviewed changes
Copy link
Contributor

@kbeker kbeker left a comment

Choose a reason for hiding this comment

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

Ready to merge

@kbeker kbeker merged commit ef4534f into master Apr 1, 2019
@kbeker kbeker deleted the feature-support-markdown-in-report-description branch April 1, 2019 15:17
@Karrp Karrp mentioned this pull request Apr 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maciejSamerdak maciejSamerdak maciejSamerdak left review comments

@MartynaAnnaGottschling MartynaAnnaGottschling Awaiting requested review from MartynaAnnaGottschling

@Karrp Karrp Awaiting requested review from Karrp

@rwrzesien rwrzesien Awaiting requested review from rwrzesien

+1 more reviewer

@kbeker kbeker kbeker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Szymiks Szymiks

Labels

feature New feature

Projects

None yet

Milestone

v0.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@Szymiks @rwrzesien @maciejSamerdak @kbeker @mszymczak94