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

Don't polute the page with useless javascript #4843

Merged
merged 11 commits into from Nov 2, 2014
Merged

Don't polute the page with useless javascript #4843

merged 11 commits into from Nov 2, 2014

Conversation

dgrammatiko
Copy link
Contributor

This is a quality control patch

When #4694 was committed there were some things done wrong due to

  1. There were some inconsistencies in the admin area namely Disable cpanel and user menu when editing #4694 and Disable menu when editing a message #4788
  2. The initial proposal actually needed javascript to function.

As none of the above exist anymore we don’t need the javascript part as well.

Results

There shouldn’t be any visual or functional changes

Test

Log in to the admin area
Create a new article
Is the joomla icon and the cog icon on the navigation disabled and not functional?
If yes test is successful

@dgrammatiko
Copy link
Contributor Author

@Bakual is this broken now?

@Bakual
Copy link
Contributor

Bakual commented Oct 23, 2014

@dgt41 Yep, looks like you wanted to update your branch with latest staging? But it applied all commits after your own one, which will mess up the branch.
There are two ways to properly update a branch:

  • Merge staging into your branch by creating a "merge-commit". This sort of puts all changes from staging into a single commit and applies it to your branch. It's what we do with 3.4-dev to update it with the latest changes from staging.
  • You can rebase your branch. This way Git reverts your own commits, then fast-forwards your branch to current staging by applying each single commit and after that it applies your own commits again on top. It makes very nice PRs but is harder to do and you should never do that on a branch where multiple people are working on (because you change the Git history). For PRs this is usually fine to do since most of the time only yourself is working with it.

@dgrammatiko
Copy link
Contributor Author

@Bakual Is this ok now or do I have to fix it?

@Bakual
Copy link
Contributor

Bakual commented Oct 31, 2014

The commits still look a bit funny, but the resulting diff (https://github.com/joomla/joomla-cms/pull/4843/files) looks about correct. So it will work fine.

@dgrammatiko
Copy link
Contributor Author

@infograf768 JM can you take a look at this one? This actually is a better implementation of #4694 (now everything is done server side so no javascript code is required). It already got one successful test...

@infograf768
Copy link
Member

This works.
I suggest to change to

<a class="admin-logo <?php echo ($hidden ? 'disabled' : ''); ?>" <?php echo ($hidden ? '' : 'href="' . $this->baseurl . '"'); ?>><span class="icon-joomla"></span></a>

and further down

<a class="<?php echo ($hidden ? ' disabled' : 'dropdown-toggle'); ?>" data-toggle="<?php echo ($hidden ? '' : 'dropdown'); ?>" <?php echo ($hidden ? '' : 'href="#"'); ?>><span class="icon-cog"></span>

this to be consistent in the code style.

@dgrammatiko
Copy link
Contributor Author

@infograf768 Thanks! It’s a lot better now

@infograf768
Copy link
Member

One more test

@Fedik
Copy link
Member

Fedik commented Nov 2, 2014

test
works good for me

@infograf768 infograf768 added this to the Joomla! 3.3.7 milestone Nov 2, 2014
@infograf768
Copy link
Member

Thanks. Merging

infograf768 added a commit that referenced this pull request Nov 2, 2014
Don't polute the page with useless javascript
@infograf768 infograf768 merged commit a4ddd69 into joomla:staging Nov 2, 2014
@dgrammatiko dgrammatiko deleted the Remove_jsscriptcode_isis branch November 8, 2014 23:51
@mbabker mbabker modified the milestones: Joomla! 3.3.7, Joomla! 3.4.0 Nov 22, 2014
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