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

Modularize PMA Navigation JS files #14468

Merged
merged 5 commits into from Jul 30, 2018

Conversation

Projects
None yet
2 participants
@Piyush3079
Copy link
Contributor

Piyush3079 commented Jul 8, 2018

This pr contains the modular code for PMA Navigation and the file names were changed in php files so that new modular code can be loaded rather than old code files.

This pr is in continuation of Mod_Js_Server_Status. Before reviewing this pr, review Mod_Js_Server_Status and merge and then compare with this pr.

Before submitting pull request, please check that every commit:

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 8, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (gsoc-js-refactoring@ed6f106). Click here to learn what that means.
The diff coverage is 15.2%.

@@                  Coverage Diff                   @@
##             gsoc-js-refactoring   #14468   +/-   ##
======================================================
  Coverage                       ?   49.93%           
  Complexity                     ?    14435           
======================================================
  Files                          ?      504           
  Lines                          ?    68972           
  Branches                       ?        0           
======================================================
  Hits                           ?    34441           
  Misses                         ?    34531           
  Partials                       ?        0

@devenbansod devenbansod changed the base branch from master to gsoc-js-refactoring Jul 28, 2018

@devenbansod devenbansod changed the title PMA Navigation modularized Modularize PMA Navigation JS files Jul 28, 2018

@devenbansod devenbansod self-assigned this Jul 28, 2018

@devenbansod devenbansod self-requested a review Jul 28, 2018

@devenbansod

This comment has been minimized.

Copy link
Member

devenbansod commented Jul 28, 2018

Please rebase and squash

@Piyush3079 Piyush3079 force-pushed the Piyush3079:Mod_Js_Navigation branch 12 times, most recently from 57f5b34 to c17c254 Jul 28, 2018

Piyush3079 added some commits May 14, 2018

Import highlight_sql module in show_ajax_messages.
Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>
Navigation revamped, navigation functions separated from main file. M…
…ore file names added in ajax and index.

Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>

@Piyush3079 Piyush3079 force-pushed the Piyush3079:Mod_Js_Navigation branch from c17c254 to 383b9c9 Jul 29, 2018

@Piyush3079
Copy link
Contributor Author

Piyush3079 left a comment

General comments to increase the understandability of the pr

@@ -13,6 +13,11 @@
// Send correct type:
header('Content-Type: text/javascript; charset=UTF-8');
// Preventing caching of this file

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 29, 2018

Author Contributor

Remove caching of this file to prevent storing of parameters in cache

@@ -271,388 +271,6 @@ function traverseNavigationForPaths () {
return params;
}

/**

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 29, 2018

Author Contributor

Remove events from js/navigation.js to avoid duplicate events and triggering of events twice

@@ -586,8 +589,8 @@ export let AJAX = {
* @todo This condition is to be removed once all the files are modularised
*/
if (checkNewCode(file)) {
var fileImports = ['server_privileges', 'server_databases', 'server_status_advisor',
'server_status_processes', 'server_status_variables',];
var fileImports = ['server_privileges', 'server_databases', 'error_report', 'navigation', 'server_status_advisor',

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 29, 2018

Author Contributor

Navigations and error report files whitelisted for ajax loading

@@ -4,6 +4,97 @@
* @returns void

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 29, 2018

Author Contributor

File containing the functions related to navigation which can be imported in files.

/**
* Adding common files for every page
*/
for (let i in files.global) {

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 29, 2018

Author Contributor

For loading common files required on every page.

@@ -0,0 +1,513 @@
/* vim: set expandtab sw=4 ts=4 sts=4: */

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 29, 2018

Author Contributor

Main navigation file containing the events related to navigation which are copied from js/navigations.js and modified accordingly for use with modular code.

@@ -183,13 +183,13 @@ private function _addDefaultScripts(): void
$this->_scripts->addFile('messages.php', array('l' => $GLOBALS['lang']));
$this->_scripts->addFile('common_params.php', array('l' => $GLOBALS['lang']));
$this->_scripts->addFile('vendors~index_new.js');

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 29, 2018

Author Contributor

vendor files are added in webpack to create a common bundle for vendor files required on every page like jquery, sprintf etc.

};
var plugins = [
new webpack.optimize.OccurrenceOrderPlugin(),
new webpack.HotModuleReplacementPlugin(),
new webpack.NamedModulesPlugin(),
new webpack.NoEmitOnErrorsPlugin()
new webpack.NoEmitOnErrorsPlugin(),
new plugin()

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 29, 2018

Author Contributor

Bundle analyzer plugin to run bundle analyzer in development mode.

@@ -47,6 +52,28 @@ export default [{
path: path.resolve(__dirname, 'js/dist'),
publicPath: PUBLIC_PATH
},
optimization: {

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 29, 2018

Author Contributor

Default configuration for chunks generated by dynamic imports.

@devenbansod
Copy link
Member

devenbansod left a comment

Looks good to me. Tested locally, works fine.

Some minor comments added.

@@ -271,388 +271,6 @@ function traverseNavigationForPaths () {
return params;
}

/**

This comment has been minimized.

@devenbansod

devenbansod Jul 29, 2018

Member

Wouldn't this complete file get deleted ultimately?

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 29, 2018

Author Contributor

Once all the files get revamped then only we will be able to delete this complete files because functions defined in this file are going to be used in many other files which are not revamped yet.

@@ -1,5 +1,8 @@
import path from 'path';
import webpack from 'webpack';
import BundleAnalyzerPlugin from 'webpack-bundle-analyzer';

let plugin = BundleAnalyzerPlugin.BundleAnalyzerPlugin;

This comment has been minimized.

@devenbansod

devenbansod Jul 29, 2018

Member

This naming could/should be improved.

@@ -183,13 +183,13 @@ private function _addDefaultScripts(): void
$this->_scripts->addFile('messages.php', array('l' => $GLOBALS['lang']));
$this->_scripts->addFile('common_params.php', array('l' => $GLOBALS['lang']));
$this->_scripts->addFile('vendors~index_new.js');

This comment has been minimized.

@devenbansod

devenbansod Jul 29, 2018

Member

Why was it not needed previously? and is only needed for this file?

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 29, 2018

Author Contributor

Its not file specific. There is slight change in webpack configuration which is generating a combined bundle of vendor files needed on every page. So this files is the bundled vendor file.

@@ -37,6 +37,7 @@
"del-cli": "^1.1.0",

This comment has been minimized.

@devenbansod

devenbansod Jul 29, 2018

Member

I get the following warning when I run yarn install:
warning " > babel-loader@7.1.5" has unmet peer dependency "babel-core@6".

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 29, 2018

Author Contributor

This is due to not including babel-core package but this will not have effect as we have installed babel-cli instead of babel-core.

Piyush3079 added some commits Jun 13, 2018

File names changed for revamped javascript files in php files.
Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>
Fallback for new Modularize navigation. Both old and new code include…
…d in Header.php

Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>
Remove caching of common_params.php to avoid stoing token in cache on…
… login and logout again.

Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>

@Piyush3079 Piyush3079 force-pushed the Piyush3079:Mod_Js_Navigation branch from 383b9c9 to 9727a94 Jul 29, 2018

@devenbansod
Copy link
Member

devenbansod left a comment

Looks good to me. Tested locally, works fine.

Moving ahead to unblock next PR.

TODO: @Piyush3079 to revisit renaming of files, folders, functions, classes once all files are revamped.

@devenbansod devenbansod merged commit 00e9049 into phpmyadmin:gsoc-js-refactoring Jul 30, 2018

1 of 2 checks passed

Codacy/PR Quality Review Codacy was unable to analyse your pull request.
Details
DCO All commits have a DCO sign-off from the author
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.