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

Remove font size feature #13925

Merged
merged 25 commits into from Apr 14, 2018
Merged

Remove font size feature #13925

merged 25 commits into from Apr 14, 2018

Conversation

shucon
Copy link
Contributor

@shucon shucon commented Jan 11, 2018

#13919
Signed-off-by: Saksham Gupta shucon01@gmail.com

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

#13919
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
@codecov
Copy link

codecov bot commented Jan 11, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@1421722). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #13925   +/-   ##
=========================================
  Coverage          ?   52.89%           
  Complexity        ?    14326           
=========================================
  Files             ?      492           
  Lines             ?    63665           
  Branches          ?        0           
=========================================
  Hits              ?    33676           
  Misses            ?    29989           
  Partials          ?        0

@MauricioFauth
Copy link
Member

Apparently you just remove the font size form. The font size feature goes beyond that.

Signed-off-by: Saksham Gupta <shucon01@gmail.com>
@shucon
Copy link
Contributor Author

shucon commented Jan 23, 2018

@MauricioFauth is this good?

@MauricioFauth
Copy link
Member

In addition to removing the form, I think removing the FontSize configuration directive is also necessary.

Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
@devenbansod
Copy link
Member

Hi @shucon I tried restarting the selenium job but the same test PhpMyAdmin\Tests\Selenium\SettingsTest::testHideDatabase failed again. Attached is the screen recording of them same.

I'd recommend you to try running the test locally (or run down the flow manually), if it fails that'd mean that your changes in this PR unknowingly seem to have broken the flow being tested in the above test or the test has been broken since a last few commits but no one caught it.

bs-video-logs-use-2-

@devenbansod
Copy link
Member

Also, I can see an error on the settings page saying: 'Field FontSize has no type' which seems related to this PR.

Signed-off-by: Saksham Gupta <shucon01@gmail.com>
@shucon
Copy link
Contributor Author

shucon commented Jan 29, 2018

@devenbansod both the issues are fixed on my side , can you please confirm the same .

);

//test getFontsizeOptions for "em" unit
$fontsize = $GLOBALS['PMA_Config']->get('FontSize');
Copy link
Member

Choose a reason for hiding this comment

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

There are more places where the FontSize setting is used with the Config class.

ufosaga and others added 6 commits February 9, 2018 10:13
Signed-off-by: Thomas Wong <ufosaga@gmail.com>
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Fix textarea failed to popup (issue #13518)
@MauricioFauth
Copy link
Member

This pull request is incomplete, there are still several places where the FontSize configuration is used.

Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
@shucon
Copy link
Contributor Author

shucon commented Mar 2, 2018

@MauricioFauth is this sufficient?

@@ -14,9 +14,6 @@
?>
/******************************************************************************/
/* general tags */
html {
font-size: <?php echo $theme->getFontSize(); ?>
}
Copy link
Member

Choose a reason for hiding this comment

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

You need to keep the font-size property here.

@@ -1191,13 +1191,7 @@ public function getSource()
*/
public function getThemeUniqueValue()
Copy link
Member

Choose a reason for hiding this comment

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

You should update the doc for this method to reflect the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find anything related to this in the doc, please help.

Copy link
Member

Choose a reason for hiding this comment

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

I'm referring to the comment block just above the method.

@MauricioFauth MauricioFauth self-assigned this Mar 15, 2018
@MauricioFauth MauricioFauth added this to the 5.0.0 milestone Mar 15, 2018
@MauricioFauth
Copy link
Member

I don't think it's a good idea to include this in 4.8 branch. This is a major change and should only be included in 5.0 branch.

Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
@shucon
Copy link
Contributor Author

shucon commented Mar 15, 2018

@MauricioFauth everything good now?

doc/config.rst Outdated
:default: '82%'

Font size to use, is applied in CSS.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be simply removed. See this example: 8cc6de4#diff-5a2f160c43a637ce34c72d79c769e4cf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, got it! I'll fix it.

Signed-off-by: Saksham Gupta <shucon01@gmail.com>
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
doc/config.rst Outdated
@@ -3232,6 +3232,11 @@ Theme manager settings

:type: string
:default: '82%'

.. deprecated:: 4.8.0
Copy link
Member

Choose a reason for hiding this comment

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

Please, change to 5.0.0. Thanks!

Signed-off-by: Saksham Gupta <shucon01@gmail.com>
@MauricioFauth MauricioFauth merged commit d701f6f into phpmyadmin:master Apr 14, 2018
@MauricioFauth
Copy link
Member

Merged! Thank you for your contribution and your patience with the various requests for change.

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

4 participants