Skip to content
This repository was archived by the owner on Sep 25, 2019. It is now read-only.

Hiepluong dev#61

Closed
hiepluonghlv wants to merge 3 commits intoCoursemology:developmentfrom
hiepluonghlv:hiepluong_dev
Closed

Hiepluong dev#61
hiepluonghlv wants to merge 3 commits intoCoursemology:developmentfrom
hiepluonghlv:hiepluong_dev

Conversation

@hiepluonghlv
Copy link
Copy Markdown
Contributor

improve sidebar task, using jQuery ajax for all actions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This kind of code can be easily broken, what if we change the layout of the sidebar item. I think, a better way is, you give the sidebar li item an id (which would be the item name), then for remove and reorder, you don't need this ugly parent().parent() thing. Also, you can have a span wrapper around the name of the sidebar item, so to update name, you just find this span element and change it's content. yeah ?

@raymondtangsc
Copy link
Copy Markdown
Contributor

Good work overall :)
To improve:

  1. validation: Name and order can't be empty, if empty, instead of updating, just bring back the old value. Order has to be an number.
  2. Use drag and drop to order the items, so don't need the order column anymore.
  3. Levels and Achievement isn't belong to the sidebar nav items. You can use a different handler to handle it (different method in controller, different js method), since to handle it specially, you have several if and else statements. Handle separately will leads to more code, but cleaner, and less coupling.

hiepluonghlv added a commit to hiepluonghlv/coursemology.org that referenced this pull request Aug 14, 2014
edit name, edit display/hide for students, remove items, add items and
"Levels and Achievement" using ajax; drag & drop for ordering; fixed Coursemology#61
and Coursemology#63
@hiepluonghlv hiepluonghlv deleted the hiepluong_dev branch August 20, 2014 08:00
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.

2 participants