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

Django - fix up model diagram to match code #4505

Merged
merged 2 commits into from
Apr 29, 2021

Conversation

hamishwillee
Copy link
Collaborator

Fixes #4495

Adds borrower and imprint to the bookinstance in UML. Note that this replaces the PNG image as an SVG to ease future editing (i.e. via inkscape)

@github-actions
Copy link
Contributor

github-actions bot commented Apr 26, 2021

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

LGTM; let's merge this.

@chrisdavidmills
Copy link
Contributor

Ah. It wants you to run --save-compression on the SVG.

@hamishwillee
Copy link
Collaborator Author

@chrisdavidmills Isn't that a bug? How can you compress a vector diagram?

@chrisdavidmills
Copy link
Contributor

@chrisdavidmills Isn't that a bug? How can you compress a vector diagram?

LOL, that's a good point! Can you file an issue on Yari?

@peterbe
Copy link
Contributor

peterbe commented Apr 27, 2021

This topic first came up in mdn/yari#1678
In the case of a little .svg file, it might feel academic and arbitrary. But the topic is interesting. It could be a binary Photoshop source/project file. So it might be smart to upload the original/source files somewhere so if the article author's laptop is run over by a bus.

@peterbe
Copy link
Contributor

peterbe commented Apr 27, 2021

@chrisdavidmills Isn't that a bug? How can you compress a vector diagram?

A vector diagram is an XML file. There are whitespace and quotations inside that can be optimized.
So to make this PR pass you'll have to run the compress command that the error hints about.

@Ryuno-Ki
Copy link
Collaborator

Plus, tools like Photoshop tend to add id attributes for layers that are not used for anything within the SVG.
Those tools might not catch these instances.

@hamishwillee
Copy link
Collaborator Author

Yes exactly - as per mdn/yari#1678 the problem I am trying to solve in this case is managing source files to ease editing. I did it in this case because I could no longer modify the png manually - so created in lucidchart, and was planning to do future edits in inkscape.

On Friday I will test if this is still editable after minification. If so that will fix this case. IMO this is still worth discussing/resolving though.

@peterbe
Copy link
Contributor

peterbe commented Apr 28, 2021

Yes exactly - as per mdn/yari#1678 the problem I am trying to solve in this case is managing source files to ease editing. I did it in this case because I could no longer modify the png manually - so created in lucidchart, and was planning to do future edits in inkscape.

I think it's a worthwhile discussion to have.
I know nothing about Lucidchart or Inkscape but I've seen it many times where an employee built some graph, exported a PNG (or something), puts that into project. Then time passes and that exported graph is now out of date but that employee has left the project.
So I'm interested in solutions to that problem. A solution that is sustainable but can never bloat the git repo with files that aren't relevant for building the site.
Nothing urgent, but ideas welcome.

@Ryuno-Ki
Copy link
Collaborator

Perhaps worth to pick this situation up in mdn/yari#3652, too?

@hamishwillee
Copy link
Collaborator Author

@chrisdavidmills I have compressed the svg image and merged.
FYI The flaw checker was showing an error because learn docs all have a list of modules in the set at the end. I have fixed this up by making the current page link into bold. This is better anyway since it provides the context of the current page in the list of modules.

@hamishwillee hamishwillee merged commit 59ae11e into mdn:main Apr 29, 2021
@hamishwillee hamishwillee deleted the django_model_fix branch April 29, 2021 23:55
@hamishwillee
Copy link
Collaborator Author

@peterbe Will continue the discussion #4505 (comment) elsewhere. But in summary, I fully agree w.r.t. image compression being important - these things can quickly get out of hand.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UML for Django Models tutorial is out of date
4 participants