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 inline styles from Relation.php #14141

Merged
merged 2 commits into from May 3, 2018

Conversation

mohitjawanjal
Copy link

@mohitjawanjal mohitjawanjal commented Mar 28, 2018

Related to #12262

Signed-off-by: Mohit Jawanjal mohit.jawanjal@imnica.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

Signed-off-by: Mohit Jawanjal <mohit.jawanjal@imnica.com>
@@ -101,16 +101,16 @@ public function getRelationsParamDiagnostic(array $cfgRelation)
$retval = '<br>';

$messages = array();
$messages['error'] = '<span style="color:red"><strong>'
$messages['error'] = '<span class="span_color_red"><strong>'
Copy link
Member

Choose a reason for hiding this comment

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

You can use the caution class here. See:

span.caution {
color: #FF0000;
}

. _pgettext('Correctly working', 'OK')
. '</strong></span>';

$messages['enabled'] = '<span style="color:green">' . __('Enabled') . '</span>';
$messages['disabled'] = '<span style="color:red">' . __('Disabled') . '</span>';
$messages['enabled'] = '<span class="span_color_green">' . __('Enabled') . '</span>';
Copy link
Member

Choose a reason for hiding this comment

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

It would be interesting to use another class name instead of span_color_green, for example: is_enabled or success. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Yup, success would be good. I'll look around for similar classes from the next time :-)

Copy link
Member

Choose a reason for hiding this comment

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

Great! Once you submit the new changes, we can merge this pull request.

Signed-off-by: Mohit Jawanjal <mohit.jawanjal@imnica.com>
@mohitjawanjal
Copy link
Author

Sorry my git kung-fu is not very good today. :( Going to re-submit it on a different PR. @MauricioFauth Do you think creating a new branch would be a good idea or should I work with master?

@@ -3434,4 +3437,4 @@
/* side menu */
#name-panel {
overflow:hidden;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the new line at the end of file again?

@@ -3683,4 +3686,4 @@
/* side menu */
#name-panel {
overflow:hidden;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the new line at the end of file again?

@MauricioFauth
Copy link
Member

@mohitjawanjal You can keep the master branch in this PR, but it is recommended to use a separate branch.

@@ -1026,6 +1026,9 @@
span.caution {
color: #FF0000;
}
span.success {
color: green;
Copy link
Member

Choose a reason for hiding this comment

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

This line contains trailing whitespace. Could you remove it?

@MauricioFauth MauricioFauth merged commit 3178fac into phpmyadmin:master May 3, 2018
@MauricioFauth
Copy link
Member

Merged, thanks for your contribution!

@MauricioFauth MauricioFauth self-assigned this May 3, 2018
@MauricioFauth MauricioFauth added this to the 5.0.0 milestone May 3, 2018
MauricioFauth added a commit that referenced this pull request May 3, 2018
Signed-off-by: Maurício Meneghini Fauth <mauriciofauth@gmail.com>
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

3 participants