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

Fix invalid inlining #34234

Merged
merged 1 commit into from Jun 14, 2016
Merged

Fix invalid inlining #34234

merged 1 commit into from Jun 14, 2016

Conversation

GuillaumeGomez
Copy link
Member

r? @steveklabnik

So to put a context. @nox found an issue on the generated doc:

screenshot from 2016-06-11 19-53-38

So as you can see, the two variants are on the same where they shouldn't. I found out that the issue is also on structs:

screenshot from 2016-06-11 19-53-31

And so such is the result of the PR:

screenshot from 2016-06-12 01-15-21
screenshot from 2016-06-12 01-15-24

@steveklabnik
Copy link
Member

Do we know what change caused this regression? I'd rather undo whatever broke it then add more stuff on top.

@GuillaumeGomez
Copy link
Member Author

I have no idea. But it seems strange that it worked.

@est31
Copy link
Member

est31 commented Jun 12, 2016

@GuillaumeGomez see my comment here

@GuillaumeGomez
Copy link
Member Author

@est31: It matches what I found, so that's why I think just fixing it like this might be better.

@sanxiyn
Copy link
Member

sanxiyn commented Jun 13, 2016

I believe this was caused by #33867, cc @oli-obk. Looking at diff, struct field rendering was changed from table (which does linebreaking) to span (which does not).

@GuillaumeGomez
Copy link
Member Author

@sanxiyn: In this PR, it's still a table.

@sanxiyn
Copy link
Member

sanxiyn commented Jun 13, 2016

@GuillaumeGomez I don't understand what you are saying. I see no <table> in diff here, and I see <table> in diff in #33867.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jun 13, 2016

Oh sorry. I thought you said that #33867 removed the <table>.

Anyway, I don't understand why we'd need a <table> in there. It's better with <span>.

@sanxiyn
Copy link
Member

sanxiyn commented Jun 13, 2016

@steveklabnik asked what change caused this regression, so I answered it is #33867. Yes, I am saying that #33867 removed <table> in struct field case.

@nox
Copy link
Contributor

nox commented Jun 13, 2016

It should probably be a <ul> IMO.

Edit: s/ol/ul/

@GuillaumeGomez
Copy link
Member Author

@nox: It'll be part of a big HTML/CSS refactor.

@steveklabnik
Copy link
Member

Okay. I think, given all this, adding a small hack is better, given that the regression has already landed. This isn't the worst thing in the world.

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 14, 2016

📌 Commit 7cd8912 has been approved by steveklabnik

@bors
Copy link
Contributor

bors commented Jun 14, 2016

⌛ Testing commit 7cd8912 with merge be8bd82...

bors added a commit that referenced this pull request Jun 14, 2016
Fix invalid inlining

r? @steveklabnik

So to put a context. @nox found an issue on the generated doc:

![screenshot from 2016-06-11 19-53-38](https://cloud.githubusercontent.com/assets/3050060/15987898/f7341de0-303b-11e6-9cd7-f2a6df423ee7.png)

So as you can see, the two variants are on the same where they shouldn't. I found out that the issue is also on structs:

![screenshot from 2016-06-11 19-53-31](https://cloud.githubusercontent.com/assets/3050060/15987900/0f66c5de-303c-11e6-90fc-5e49d11b6903.png)

And so such is the result of the PR:

![screenshot from 2016-06-12 01-15-21](https://cloud.githubusercontent.com/assets/3050060/15987904/19d9183c-303c-11e6-91c1-7c3f1163fbb0.png)
![screenshot from 2016-06-12 01-15-24](https://cloud.githubusercontent.com/assets/3050060/15987905/1b5d2db0-303c-11e6-8f43-9a8ad2371007.png)
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

6 participants