Skip to content

Conversation

@milmazz
Copy link
Member

@milmazz milmazz commented Jun 8, 2015

This is a Work in Process, there are a lot of things to fix at this point:

  • Fix unit tests
  • Review the CSS styles changes, delete useless rules
  • Review the breadcrumbs generation
  • Fix the possible conflict after Highlight.js improvements #212 gets merged
  • Fix the index.html to avoid the frameset.
  • Improve the section below the #sidebar section in module_template
  • Generate valid JSON for sidebar_items.js
  • Improve CSS integration
  • Remove frames
  • Add an initial media query for print media
  • Improve the JS style code and documentation
  • Add meta tags x-ua-compatible, viewport and generator
  • Delete target parameter from a element
  • Add normalize.css
  • Merge old full_list.css with style.css
  • Add sidebar template
  • Merge full_list.js into app.js
  • Update jQuery v2.1.4

This effort will try to fix the issue #175

Any comments or suggestions are more than welcome. I'll be grateful for any feedback.

@milmazz milmazz force-pushed the frames branch 3 times, most recently from 04450b8 to 745fa62 Compare June 10, 2015 02:56
This is a Work in Process, there are a lot of things to fix at this
point:

 - [x] Fix tests
 - [x] Review the changes made to the CSS styles
 - [x] Review the breadcrumbs
 - [x] Fix the possible conflict after elixir-lang#212 gets merged
 - [x] Fix the index.html to avoid the frameset.
 - [x] Improve the section below the `#sidebar` section in `module_template`
 - [x] Generate valid JSON for `sidebar_items.js`
 - [x] Improve CSS integration
 - [x] Remove frames
 - [x] Add an initial media query for `print` media
 - [x] Improve the JS style code and documentation
 - [x] Add meta tags `x-ua-compatible`, `viewport` and `generator`
 - [x] Delete `target` parameter from a element
 - [x] Add normalize.css
 - [x] Merge old full_list.css with style.css
 - [x] Add sidebar template
 - [x] Merge full_list.js into app.js
 - [x] Update jQuery v2.1.4

This effort will try to fix the issue elixir-lang#175
@rrrene
Copy link

rrrene commented Jun 10, 2015

👍 looks good!

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you assigning to a built in variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

I realised this was in the old code, but it's very bad JavaScript.. just do something lke

function escapeText(text) {
  return text.replace(/[\-\[\]{}()*+?.,\\\^$|#\s]/g, "\\$&");
}

instead

Copy link
Member Author

Choose a reason for hiding this comment

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

@dignifiedquire Thanks for your feedback, as you mentioned it was old code, but I already applied your suggestion.

@dignifiedquire
Copy link
Contributor

Great stuff !

@josevalim
Copy link
Member

This is awesome, let me give it a try locally. :)

@josevalim
Copy link
Member

@milmazz this looks great. Some feedback:

  1. Things look a bit off on my machine. Do you have any idea what I am doing wrong?

    screen shot 2015-06-10 at 14 20 38

  2. the background of code snippets has changed (we decided on yellow to match elixir-lang.org). see the original here: http://elixir-lang.org/docs/master/elixir/

  3. The goal of removing the iframes was so we could link to pages like "Agent.html" directly. However it does not seem to work in the docs generated by this PR. Thoughts? :)

@milmazz
Copy link
Member Author

milmazz commented Jun 10, 2015

@josevalim Thanks for your feedback, It looks really awful in your setup. I think the PR cover the things that you mentioned in 2 and 3.

Maybe I'm testing in the wrong way:

$ ls
elixir ex_doc
$ ex_doc/bin/ex_doc "Elixir" "1.1.0-dev" elixir/lib/elixir/ebin -u "https://github.com/elixir-lang/elixir" --readme elixir/README.md -o doc
$ cd doc
$ python -mSimpleHTTPServer

capture

@josevalim
Copy link
Member

It was my fault, sorry. Thanks for the snippet, it is all good now! Some final suggestions:

  1. Let's keep "Modules" / "Exceptions" / "Protocols" as links instead of buttons. The buttons look very weird on both Safari and Chrome. Links are easier to style. Let's also remove the horizontal line below overview and remove the padding and increased font on the project name. The reason is that Elixir is a short project name but we can have longer ones and they won't look good with the increasing padding and font from the new version. Basically, let's keep the top upper corner as before as it was before :)
  2. Let's remove the code that redirects #!Kernel.html to Kernel.html as we no longer need it

Then we can ship it! :D

Fix some differences in the CSS style after merge.
@milmazz
Copy link
Member Author

milmazz commented Jun 10, 2015

@josevalim 1. Already changed the navigation area to a elements instead of button. Regarding the styles of the upper-left corner was a problem after merging the CSS files (full_list.css + style.css), I think is already fixed 😃
2. Can you please point in the right direction?, which code is that? js or Elixir 😕

Please let me know if you need something else.

josevalim added a commit that referenced this pull request Jun 10, 2015
WIP: Frameless docs + New Menu
@josevalim josevalim merged commit 0b72818 into elixir-lang:master Jun 10, 2015
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@josevalim
Copy link
Member

2 has been fixed already too. Sorry, I am not on top of my game today. :)

I have merged this, thank you so much!

@josevalim
Copy link
Member

I will open up issues for other bugs I find. :)

@milmazz
Copy link
Member Author

milmazz commented Jun 10, 2015

@josevalim No problem, thanks for your help and your feedback.

According to you, there is another issue with high priority:question:

@josevalim
Copy link
Member

I have opened two issue reports and copied you on it. I think most of the issues can be ironed out quickly, so I have decided to merge this so we can get more feedback. :)

@eksperimental
Copy link
Contributor

sorry for the late reply, I just wanted to make one comment, but I haven't had the chance to do it before.
first of all thanks @milmazz for the hard work you are taking on ex_doc,

I have noticed there has been a bug in ex_doc now after this PR (#229) and I realized that JQuery has been updated, I had that in mind and I since we were running a very old version, it was recommended to do a migration process.
you can read about it in this Issue i created in my own repository,
https://github.com/eksperimental/ex_doc/issues/18

Have you done this? I think we should do it to avoid any further issues in case you have not.
also @milmazz if you are looking for ideas to contribute to, you can have a look at my personal Issue tracker,
https://github.com/eksperimental/ex_doc/issues
feel free to grab anything from there as I'm not working in any of them at the moment.

cheers

@milmazz
Copy link
Member Author

milmazz commented Jun 21, 2015

@eksperimental Thank you, I'm really having fun 😃

As I mentioned to @josevalim, I've been able to reproduce this bug in the stable version too. Right now the bug is already fixed and merged into the master branch. Specifically the problem was the following:

$('#search input').bind("keyup search reset change propertychange input paste", function (evnt) {

The change event is fired every time the input element lose its focus, so, the previous version was executing another search even when was not required.

TBH, I didn't checked jquery-migrate for this jQuery migration (the previous jQuery version was so old that I forgot about jquery-migrate, I haven't use it in a while, hehehe), but I already created a branch including it and I don't see any warning message yet, regardless of that I'll keep doing some tests.

As a side note, the jslint tool was really useful in this case, with jslint I've been able to detect some issues in the JS code, all of which were fixed in previous PR. Anyway, as I mentioned previously I'll review the JS code again.

Finally, thanks for your suggestions, I'll look into your personal issue tracker for ExDoc looking for new ideas.

Happy hacking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants