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

Ticket 15109 #4787

Closed
wants to merge 1 commit into from

Conversation

@YasaraPeiris
Copy link

commented Apr 4, 2017

#Checklist:

Correct branch: master for new features
Tests pass
Code follows coding guidelines: master
Commit follows commit message format
PHPBB3-15109

https://tracker.phpbb.com/browse/PHPBB3-15109

In here I have added a smiley dropdown toeditor to save some screen
space. Mainly I have modified posting_editor.html file to remove the
existing side smilebox container. Then I have added a new file called
smiley_dropdown.html display smiley dropdown. This newly created file is
included to the posting_editor.html. After that I did necessarry
modifications to styles in colour.css, forms.css and common.css. All the
modifications are commented for easy identification.

View of the smiley dropdown

image

@DavidIQ

This comment has been minimized.

Copy link
Member

commented Apr 4, 2017

Did you, for some reason, run a refactoring tool on the two CSS files? There's a lot of changes that are irrelevant to this pull request and shouldn't be here and makes reviewing the applicable changes very difficult, if not impossible. Things like changing double quotes to single quotes, re-arranging of style properties, adding of extra properties for browser compatibility, etc., which are not needed for this enhancement and have caused the build to fail as they are against the style linting rules set for our repository.

@YasaraPeiris

This comment has been minimized.

Copy link
Author

commented Apr 4, 2017

Sorry, Didn't get what you mean?
I changed only the style related to my modification. What do I need to do now.

@YasaraPeiris

This comment has been minimized.

Copy link
Author

commented Apr 4, 2017

I just included some style classes and changed some style to match with my modification.

@DavidIQ

This comment has been minimized.

Copy link
Member

commented Apr 4, 2017

Ah I see, it looks like your rebasing was not done properly as your CSS files are not in line with the latest version of those files. You'll need to fix that.

@YasaraPeiris

This comment has been minimized.

Copy link
Author

commented Apr 4, 2017

@@ -3,8 +3,6 @@
/* -------------------------------------------------------------- */
/* stylelint-disable selector-max-compound-selectors */
/* stylelint-disable selector-no-qualifying-type */
/* stylelint-disable declaration-property-unit-whitelist */
/* stylelint-disable declaration-property-unit-blacklist */

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 4, 2017

Member

you are not or have not achieved a proper re-base as this should not be removed. The issue is maybe that you are trying to rebase and then apply changes that you made from a 3.2 or older master version of these files which is causing conflicts. all the css changes will need to be reverted and then manually re-added to properly clean things up.

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 4, 2017

Member

also pls clsoe this duplicate #4779

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Apr 4, 2017

Author

Would you think that the issue would be solved if I revert the files? Okay, Then I'll give a try, Thanks a lot for your support.

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 4, 2017

Member

not just the css but this was just an example as posting_editor.html is old 3.2 for sure

@YasaraPeiris

This comment has been minimized.

Copy link
Author

commented Apr 4, 2017

After modifying files I couldn't see any changes when I check on the browser. The class I removed from posting editor was there. Thought it because of cache issue and I renamed one file inside phpbb\phpBB\cache\production\twig to observe the results of my modifications.

@YasaraPeiris

This comment has been minimized.

Copy link
Author

commented Apr 4, 2017

@hanakin

This comment has been minimized.

Copy link
Member

commented Apr 4, 2017

you have corrected the issues with the basing however your commit msgs are not formatted properly you will need to rebase and make sure to only perform clean changes to avoid merge and revert commits squash everything into a single commit and you still need to fix all the stylelint errors.

@YasaraPeiris

This comment has been minimized.

Copy link
Author

commented Apr 5, 2017

@hanakin

This comment has been minimized.

Copy link
Member

commented Apr 5, 2017

those just temporarily ignore those specific errors which is supposed to be there but you still need to fix the 50+ individual errors that you have as your code is not formated properly to match all the stylelint rules. I am still working on all the documentation for all the rules but for now if you read the following http://hanakin.github.io/base-l/#/codeing-guidelines and everything under the CSS section as well that is essentially the rules we follow. You should see all your errors if you have correctly setup your editor...or from the project root folder run stylelint "phpBB/styles/prosilver/theme/*.css" to see all the errors. You can also look at the travis log to see them as well...https://travis-ci.org/phpbb/phpbb/jobs/218455733#L915

@YasaraPeiris

This comment has been minimized.

Copy link
Author

commented Apr 5, 2017

@YasaraPeiris

This comment has been minimized.

Copy link
Author

commented Apr 7, 2017

@hanakin

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

did you run npm install in the project root before running that command from the same location? Also it's not just L915 that was only one of the errors on there.

@YasaraPeiris

This comment has been minimized.

Copy link
Author

commented Apr 8, 2017

@hanakin

This comment has been minimized.

Copy link
Member

commented Apr 8, 2017

The issue is related to things not being installed and setup properly in the correct places. Try deleting your node_modules folder from the project root and rerunning the npm install again from the same location the retry the stylelint command

@marc1706

This comment has been minimized.

Copy link
Member

commented Apr 9, 2017

Just on a side note, you might have to run npm install with the global flag first, e.g. npm install -g

@YasaraPeiris

This comment has been minimized.

Copy link
Author

commented Apr 10, 2017

@hanakin

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

i was having some issues with some other repos earlier last month, there was an update to node or npm that was causing some issues and I had to reinstall node delete my modules folder and re install the pkgs.


<div class="dropdown-container smiley-container" data-skip-responsive="true">
<a href="{U_VIEW_ALL_NOTIFICATIONS}" id="notification_list_button" class="dropdown-trigger">
<img src="./images/smilies/icon_e_biggrin.gif" width="16" height="16" alt="" title="Add an emticon.">

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 10, 2017

Member

this needs to no be an img use an icon from fontawsome

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 10, 2017

Member

also the title will become sr-only text and should be a language variable

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Apr 14, 2017

Author

I just needed to include one smiley for the label of the dropdown container. So used one from the emticon panel.

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 15, 2017

Member

it needs to be themed the same as the buttons around it so use fa-smile


<!-- BEGIN smiley -->
<li class="smiley-list">
<a href="#" onclick="insert_text('{smiley.A_SMILEY_CODE}', true); return false;"><img src="{smiley.SMILEY_IMG}" width="{smiley.SMILEY_WIDTH}" height="{smiley.SMILEY_HEIGHT}" alt="{smiley.SMILEY_CODE}" title="{smiley.SMILEY_DESC}" /></a>

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 10, 2017

Member

js code should never be inserted into your html use jquery and data elements to abstract it into the forum_fn.js file

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Apr 17, 2017

Author

This was used inside the code.
<!-- IF S_SMILIES_ALLOWED and .smiley --> <strong>{L_SMILIES}</strong><br /> <!-- BEGIN smiley --> <a href="#" onclick="insert_text('{smiley.A_SMILEY_CODE}', true); return false;"><img src="{smiley.SMILEY_IMG}" width="{smiley.SMILEY_WIDTH}" height="{smiley.SMILEY_HEIGHT}" alt="{smiley.SMILEY_CODE}" title="{smiley.SMILEY_DESC}" /></a> <!-- END smiley --> <!-- ENDIF -->

That's why I used this.. Do I need to insert this function to forum_fn.js file.

<!-- END smiley -->
<!-- ENDIF -->
<!-- IF S_SHOW_SMILEY_LINK and S_SMILIES_ALLOWED -->
<br /><li><a href="{U_MORE_SMILIES}" onclick="popup(this.href, 750, 350, '_phpbbsmilies'); return false;">{L_MORE_SMILIES}</a></li>

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 10, 2017

Member

give this an id (maybe#show_smileys) and abstract the js out into forum_fn.js using jquery.

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Apr 18, 2017

Author

This was also used, thought itz better to use the same way. If extracting to forum_fn.jsis better, then I'll work on that.

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 18, 2017

Member

you are overhauling or replacing the code. It makes sense to overall the whole thing cleanly rather than to perpetuate and old practices/stds. So why not use data attributes to store template vars and jquery to extract and execute the events rather than inline JS.

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Apr 18, 2017

Author

I have commited again after modifying my code.

<div id="smiley-dropdown" class="dropdown dropdown-extended" > <div class="pointer"><div class="pointer-inner"></div></div> <div class="smiley-content"> <div id="smiley-box" class="smiley-box"> <ul> <!-- EVENT posting_editor_smilies_before --> <!-- IF S_SMILIES_ALLOWED and .smiley --> <!-- BEGIN smiley --> <li class="smiley-list"> <a href="#" onclick="insertText('{smiley.A_SMILEY_CODE}')"><img src="{smiley.SMILEY_IMG}" width="{smiley.SMILEY_WIDTH}" height="{smiley.SMILEY_HEIGHT}" alt="{smiley.SMILEY_CODE}" title="{smiley.SMILEY_DESC}" /></a> </li> <!-- END smiley --> <!-- ENDIF --> <!-- IF S_SHOW_SMILEY_LINK and S_SMILIES_ALLOWED --> <br /><li><a href="{U_MORE_SMILIES}" onclick="popupSmileys(this.href)">{L_MORE_SMILIES}</a></li> <!-- ENDIF --> <!-- EVENT posting_editor_smilies_after --> <!-- IF BBCODE_STATUS --> <div class="bbcode-status"> <!-- IF .smiley --><hr /><!-- ENDIF --> {BBCODE_STATUS}<br /> <!-- IF S_BBCODE_ALLOWED --> {IMG_STATUS}<br /> {FLASH_STATUS}<br /> {URL_STATUS}<br /> <!-- ENDIF --> {SMILIES_STATUS} </div> <!-- ENDIF --> <!-- EVENT posting_editor_bbcode_status_after --> <!-- IF S_EDIT_DRAFT || S_DISPLAY_REVIEW --> <!-- IF S_DISPLAY_REVIEW --><hr /><!-- ENDIF --> <!-- IF S_EDIT_DRAFT --><strong><a href="{S_UCP_ACTION}">{L_BACK_TO_DRAFTS}</a></strong><!-- ENDIF --> <!-- IF S_DISPLAY_REVIEW --><strong><a href="#review">{L_TOPIC_REVIEW}</a></strong><!-- ENDIF --> <!-- ENDIF --> </ul> </div> </div> </div>

resize: vertical;
-webkit-transition: all 0.5s ease;
-moz-transition: all 0.5s ease;
-ms-transition: all 0.5s ease;

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 10, 2017

Member

run everything through autoprefixer as you have way too many prefixes for the proper browser support we use > 1%, last 2 versions


<div class="dropdown-container smiley-container" data-skip-responsive="true">
<a href="{U_VIEW_ALL_NOTIFICATIONS}" id="notification_list_button" class="dropdown-trigger">
<img src="./images/smilies/icon_e_biggrin.gif" width="16" height="16" alt="" title="Add an emticon.">

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 10, 2017

Member

also the title will become sr-only text and should be a language variable

<textarea <!-- IF S_UCP_ACTION and not S_PRIVMSGS and not S_EDIT_DRAFT -->name="signature" id="signature" style="height: 9em;"<!-- ELSE -->name="message" id="message"<!-- ENDIF --> rows="1" cols="76" tabindex="4" onselect="storeCaret(this);" onclick="storeCaret(this);" readonly="readonly" onkeyup="storeCaret(this);" onfocus="initInsertions();" class="inputbox">{MESSAGE}{DRAFT_MESSAGE}{SIGNATURE}</textarea>

<div class="dropdown-container smiley-container" data-skip-responsive="true">
<a href="{U_VIEW_ALL_NOTIFICATIONS}" id="notification_list_button" class="dropdown-trigger">

This comment has been minimized.

Copy link
@vinny

vinny Apr 11, 2017

Member

U_VIEW_ALL_NOTIFICATIONS > U_VIEW_ALL_SMILIES

@@ -260,9 +260,10 @@ fieldset.submit-buttons input {
vertical-align: middle;
}

/* Main message box */
/* Main message box - Edited to add smiley dropdown */

This comment has been minimized.

Copy link
@vinny

vinny Apr 11, 2017

Member

not necessary


<div class="dropdown-container smiley-container" data-skip-responsive="true">
<a href="{U_VIEW_ALL_NOTIFICATIONS}" id="notification_list_button" class="dropdown-trigger">
<img src="./images/smilies/icon_e_biggrin.gif" width="16" height="16" alt="" title="Add an emticon.">

This comment has been minimized.

Copy link
@vinny

vinny Apr 11, 2017

Member

Add an emticon. hard-coded text

@vinny

This comment has been minimized.

@YasaraPeiris

This comment has been minimized.

Copy link
Author

commented Apr 11, 2017

@@ -603,6 +603,47 @@ a.header-avatar span:after {
display: none !important;
}

/* Smiley Dropdown
---------------------------------------- */

This comment has been minimized.

Copy link
@VSEphpbb

VSEphpbb Apr 15, 2017

Member

you have a mix of tabs and spaces. please just use tabs

<!-- IF S_SMILIES_ALLOWED and .smiley -->
<!-- BEGIN smiley -->
<li class="smiley-list">
<a href="#" onclick="insertText('{smiley.A_SMILEY_CODE}')"><img src="{smiley.SMILEY_IMG}" width="{smiley.SMILEY_WIDTH}" height="{smiley.SMILEY_HEIGHT}" alt="{smiley.SMILEY_CODE}" title="{smiley.SMILEY_DESC}" /></a>

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 18, 2017

Member

you completely misunderstood what I was talking about are you familiar with jquery and dom selection

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Apr 18, 2017

Author

yeah, I'm familiar with jquery and dome selection. Do you need me to insert an id attribute here and select that inside forum_fn.js using $('#id').click(function(){ /*function to run });

I used previous way because I thought it would be good to use the same way as it was.

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 18, 2017

Member

no never assume whats there is good it was code over a decade ago ;) ... always use best practices! ie no inline js, always data attr's and #selectors via jquery for handling js. that is if its this simple obviously if it requires a complete re-write than its best left for a separate pr...

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Apr 18, 2017

Author

Thanks a lot. I was little bit afraid to changed your code completely. Now I ll do. :)

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 18, 2017

Member

just use data attributes for the vars. ie <a href="#" id="smiley-add" data-smiley-text="{smiley.A_SMILEY_CODE}">
then you can get it via get click event on $('#smiley-add') then get the code via $(this).attr('data-smiley-text')` and call insert_text using this value

<!-- END smiley -->
<!-- ENDIF -->
<!-- IF S_SHOW_SMILEY_LINK and S_SMILIES_ALLOWED -->
<br /><li><a href="{U_MORE_SMILIES}" onclick="popupSmileys(this.href)">{L_MORE_SMILIES}</a></li>

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 18, 2017

Member

not sure this is necessary any more since we are using a dropdown already just show all smiles...so remove this bit completely

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Apr 18, 2017

Author

okay, sure,,, Morevoer um little bit worried about where those variables get assigned. which means variables with curly braces. I tried to find inside the project,, But couldn't found. Can u help me with that.

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 18, 2017

Member

well this function is not needed anymore anywhere so do not worry about this one check comment on other line for how to handle that one

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Apr 18, 2017

Author

image
Could u please help with this. Suddenly it gave me this error after i synced.

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Apr 18, 2017

Author

@hanakin Do you have any idea about this. If soo could you help me with this. Sorry for bothering you. Nd no hurry.

@hanakin

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

can you please post a screen capture of what the posting page looks like after these changes...

@YasaraPeiris YasaraPeiris force-pushed the YasaraPeiris:ticket/15109 branch 4 times, most recently from c98100f to fcc186e Apr 27, 2017

@hanakin

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

you did not fix your conflicts properly you can not just save a copy of a file over top. This completely re-writes the entire file contents. You need to rebase and i would suggest you manually add the changes to common.css & colours.css back

@YasaraPeiris

This comment has been minimized.

Copy link
Author

commented Apr 28, 2017

Okay. I ll do.

@YasaraPeiris YasaraPeiris force-pushed the YasaraPeiris:ticket/15109 branch 3 times, most recently from 7866a42 to 6bac41b Apr 28, 2017

@phpbb-user phpbb-user removed the WIP 🚧 label Apr 29, 2017

@YasaraPeiris YasaraPeiris force-pushed the YasaraPeiris:ticket/15109 branch from 6bac41b to 7532914 Apr 29, 2017

.dropdown-extended .dropdown-contents.smiley-wrap {
width: 200px;
padding: 8px;
}

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 29, 2017

Member

hmm did not work, try putting the padding on .smiley-list instead then change width here to 187px.

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Apr 29, 2017

Author

okay. In the smiley list there's an exisiting padding of 2px. Shall I increase it to 8px.

@@ -603,6 +603,32 @@ a.header-avatar span:after {
display: none !important;
}

/* Smiley Dropdown---------------------------------------- */

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 29, 2017

Member

please put ----------- on a new line

{SMILIES_STATUS}
</div>
<!-- ENDIF -->
<!-- IF S_EDIT_DRAFT --><strong><a href="{S_UCP_ACTION}">{L_BACK_TO_DRAFTS}</a></strong><!-- ENDIF -->

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 29, 2017

Member

delete this line its duplicated

</div>
<!-- ENDIF -->
<!-- IF S_EDIT_DRAFT --><strong><a href="{S_UCP_ACTION}">{L_BACK_TO_DRAFTS}</a></strong><!-- ENDIF -->
<!-- IF S_DISPLAY_REVIEW --><strong><a href="#review">{L_TOPIC_REVIEW}</a></strong><!-- ENDIF -->

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 29, 2017

Member

delete this line its duplicated

@YasaraPeiris YasaraPeiris force-pushed the YasaraPeiris:ticket/15109 branch from 7532914 to 2d084ad Apr 29, 2017

cursor: pointer;
}

.smiley-wrap {

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 29, 2017

Member

so this still needs dropdown-contents apparently until the dropdowns can be re-factored

padding: 3px;
}

.smiley-add {

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 29, 2017

Member

again this still needs smiley-list-item until dropdowns are refactored

<!-- ENDIF -->
</div>
</div>

This comment has been minimized.

Copy link
@hanakin

hanakin Apr 29, 2017

Member

I thnk you have a mix of tabs and spaces through out this file please clean this all up and only use tabs!

@YasaraPeiris YasaraPeiris force-pushed the YasaraPeiris:ticket/15109 branch from 2d084ad to c74b817 Apr 29, 2017

@hanakin
Copy link
Member

left a comment

ensure the screenshot is up todate, but other than that this finally looks good

'use strict';
// View more smileys.
var a = $(this).attr('href');
popupSmileys(a);

This comment has been minimized.

Copy link
@hanakin

hanakin Sep 8, 2017

Member

this function does not exist and is not executed correctly as you need to call popup() on click. Also you have spaces instead of tabs here

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Sep 28, 2017

Author

All I need to do is replace popupSmileys(a) function with popup() ??? @hanakin

This comment has been minimized.

Copy link
@hanakin

hanakin Sep 28, 2017

Member

not sure, just make the popup work cause its not right now

This comment has been minimized.

Copy link
@hanakin

hanakin Sep 28, 2017

Member

also can you pls delete this branch https://github.com/YasaraPeiris/phpbb/tree/Ticket/15109 from your repo as its causing conflicts for me to test things

This comment has been minimized.

Copy link
@YasaraPeiris

<!-- IF S_SHOW_SMILEY_LINK and S_SMILIES_ALLOWED -->
<div class="footer">
<a href="{U_MORE_SMILIES}"><span>{L_MORE_SMILIES}</span></a>

This comment has been minimized.

Copy link
@hanakin

hanakin Sep 8, 2017

Member

you have spaces instead of tabs here

This comment has been minimized.

Copy link
@YasaraPeiris

YasaraPeiris Sep 28, 2017

Author

I'll fix this.

@hanakin

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

@Derky This is still on my todo list since she has not made the changes...Thos only thing really missing is the JS function hookup.

@Derky

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

Ok, thanks for taking it over! 😀

@hanakin

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

closing due to time constraints and BC issues easier to push as new pr

@hanakin hanakin closed this Mar 28, 2018

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.