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

Propose a new fresh UI - Tabs! #135

Closed
wants to merge 14 commits into from

Conversation

pedro-mendonca
Copy link
Contributor

The Show/Hide sections isn't very funcional.
It needs a lot of scrolling and a litte ugly.

I've maintained the whoole code, except section navigation.
It uses WordPress native css for Tabs.

Some old organization might need a cleanup, but for now I've maintained everything as is to minimize commit diffs.
I've pushed partial commits to make your work easyer to understand what changed, as the whole diff is too long to understand.

The code was adapted from a living example: WP-Maintenance-Mode tabbed admin settings

There are some issues not solved yet, but I believe you @johnclause are the one who can solve it:

After saving changes it goes back to first tab.
The original code keeps in same tab after saving, but in this case <form action="<?php echo $clean_uri;?>" method="post"> is the problem. If <form> have action="" it works like a charm.
I didn't touch this because I think it's better in your hands from this point beyond.

Please check this out, it really looks way better and works great!
:)

@pedro-mendonca pedro-mendonca changed the title Propose a new fresh UI - tabs Propose a new fresh UI - Tabs! Apr 16, 2015
@johnclause
Copy link
Member

Hi Pedro, I will play with this after 3.3 release. This seems to be a pretty big change, which I also planned to do one day, I really appreciate that you took the initiative. I thought I would have to change functions qtranxf_admin_section_start/end only for that, but it seems to have taken many more changes. I will review it in a few days. Thanks!

@pedro-mendonca
Copy link
Contributor Author

Yes, of course.
There are two options, the tabs reloading the page and the 5 forms being separate, or the tabs are just hidden and shown without reloading.
I don't really know what will you prefer, I just did a change in the shown/hidden sections using the tabs, I did no change at all in the overall structure.
To make it easier, I've made the partial commits with less changes each one.
I took out the qtransf_admin_section_start/end the show/hide beaviour and used basic jquery for the nav tabs. Then I've added the necessary divs wrapping those admin_sections. i did it INSIDE the <form> because it's just one for sections1, 2 and 3.
A lot of changes occur just because of the conde indenting, I can revert that if it helps to understand the commit diffs.
If you want I can try to improve it by puting inside those ..._start/end functions the most part of the wrapping code.

@qtranslateteam
Copy link

I did not follow through your code yet, and I am still not sure why changes
other than inside _start/end functions (including their arguments) are
needed. I thought I would keep it encapsulated by sections as it is now,
and only re-implement _start/end to show tabs. Is that possible? And I also
thought that I will not ask people to reload page to change a tab, your
second option, I guess, this is how you did it too?

On Fri, Apr 17, 2015 at 8:09 AM, Pedro Mendonça notifications@github.com
wrote:

Yes, of course.
There are two options, the tabs reloading the page and the 5 forms are
different, or the tabs are just hidden and shown without reloading.
I don't really know what will you prefer, I just did a change in the
shown/hidden sections using the tabs, I did no change at all in the overall
structure.
To make it easier, I've made the partial commits with less changes each
one.
I took out the qtransf_admin_section_start/end the show/hide beaviour and
used basic jquery for the nav tabs. Then I've added the necessary divs
wrapping those admin_sections. i did it INSIDE the

because it's
just one for sections1, 2 and 3.
If you want I can try to improve it by puting inside those ..._start/end
functions the most part of the wrapping code.


Reply to this email directly or view it on GitHub
#135 (comment)
.

@pedro-mendonca
Copy link
Contributor Author

Yes, as I said, the basic structure is the same, so, no reloading.
After looking at it again now that my work was completed, I think I can move a lot of the section wrapping to _start/end functions. I'll do this and remove the indenting for now, it was necessary for me to understand the code but now I can revert it.

I'll do some changes and add to the current commits, ok?
Meanwhile, I'll keep pushing master commits to this branch so that it won't get to far behind and at any moment it can be included in master. Because of this some more merging commits will keep apearing and the list will increase, but in the end, the commits diffs will reduce.

@qtranslateteam
Copy link

indenting

I use tab size equal to 2. Most editors have an option for tab size, which
helps a lot. Is that what you mean?

On Fri, Apr 17, 2015 at 8:31 AM, Pedro Mendonça notifications@github.com
wrote:

Yes, as I said, the basic structure is the same, so, no reloading.
Looking at it again after the work is done, I think I can move a lot of
the section wrapping to _start/end functions. I'll do this and remove the
indenting for now, it was necessary for me to understand the code but now I
can revert it.

I'll do some changes and add to the current commits, ok?
Meanwhile, I'll keep pushing master commits to this branch so that it
won't get to far behind and at any moment it can be included in master.
Because of this some more merging commits will keep apearing and the list
will increase, but in the end, the commits diffs will reduce.


Reply to this email directly or view it on GitHub
#135 (comment)
.

@johnclause
Copy link
Member

It could be easier for you to make a new fork and then to copy relevant pieces from your local copy into the new fork?

This commit will help to show up only the necessary changes to switch from current Show/Hide to Tabs navigation.
@pedro-mendonca
Copy link
Contributor Author

@johnclause
I just pulled some more commits to this.
I've kept myself within the same branch, locally I've started all over with the current master files, changed only the necessary code to switch from Hide/Show to Tabs and commited the file to ui-tabs branch.
This shows a lot of reversing changes in the particular commit pedro-mendonca@cd6a468 because it's on top of previous changes, but as I did it from scratch without the overall code cleaning, the global commit diffs that appear here are the ones I've really made, and are just the simple changes needed to switch GUI to tabs.

Summary:
Included admin container classes in _start/end
included global that contains the admin sections. There are two of them because you have also two wraps.
Included navigation tabs and necessary jquery script
Used variables to define section titles, to avoid redundancy in reusing the titles for tabs and alt text.

Please check if it's simpler now.

@pedro-mendonca
Copy link
Contributor Author

After this, I think some stuff can be deleted:

  • The function that used to Show/Hide
  • The var $section from the function _start is no longer needed as the Section Title is defined in the Navigation Tabs
  • The cookie request and function to remember which sections where shown/hidden

@johnclause
Copy link
Member

Pedro, I am sorry for the delay, I will play with this after 3.3 is out. Otherwise there will be not much new in 3.4 ;)

@pedro-mendonca
Copy link
Contributor Author

pedro-mendonca commented Apr 30, 2015 via email

@johnclause
Copy link
Member

I do not know, it still says that it can be merged automatically. I did not change the file affected much and will probably not change before merging this PR.

@pedro-mendonca
Copy link
Contributor Author

Maybe it's because ui-tabs branch isn't on top of latest commit on your master.
I'm doing an update, it will increase the list of commits above but will be updated on top of current master.

Update on top of current `master` branch
@johnclause
Copy link
Member

oops ... Now it says it cannot merge? What happened?

@pedro-mendonca
Copy link
Contributor Author

It got out of sync because of last updates, give-me a minute to sync it.

@pedro-mendonca
Copy link
Contributor Author

Ok, I see that there are recent changes in the same files, let me rearrange that and I'll notice you.

@pedro-mendonca
Copy link
Contributor Author

This PR is getting very confuse with syncing between 3 branches.
I'm closing this and making a brand new clean PR.

@pedro-mendonca pedro-mendonca deleted the ui-tabs branch May 5, 2015 23:26
@pedro-mendonca pedro-mendonca mentioned this pull request May 5, 2015
@pedro-mendonca
Copy link
Contributor Author

I've rewritten the code from scratch in a brand new ui-tabs branch and pulled it again in #153

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

Successfully merging this pull request may close these issues.

3 participants