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

Added style to col subtract button to fix issue #756 #758

Merged

Conversation

NARUDESIGNS
Copy link
Collaborator

The column subtract button in the table popup had no style due to misspelt class name. This PR solves the problem and properly adds style to the button just like the other buttons.
@TildaDares @jywarren Please help review and feedback, thank you very much!

@gitpod-io
Copy link

gitpod-io bot commented Nov 13, 2021

});
$(document).on('click', '#incCols', function() {
$("#tableCols").text( Number($("#tableCols").text()) + 1 );
});
$(document).on('click', '#decCols', function() {
$("#tableCols").text( Number($("#tableCols").text()) - 1 );
const numOfCols = Number($("#tableCols").text());
if (numOfCols > 1) $("#tableCols").text( numOfCols - 1 );
Copy link
Member

Choose a reason for hiding this comment

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

Hi @NARUDESIGNS, these changes are from another PR. Maybe you forgot to do a git stash before switching to another branch.

Copy link
Collaborator Author

@NARUDESIGNS NARUDESIGNS Nov 15, 2021

Choose a reason for hiding this comment

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

Hi @NARUDESIGNS, these changes are from another PR. Maybe you forgot to do a git stash before switching to another branch.

Hi, sorry I haven't used git stash before but I will look it up and make changes. But before I go on to do that, I need you to help me clarify. Do you mean that I have already made these changes in a different PR previously? If yes, please point me to it.
Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

@NARUDESIGNS I mean these changes were made for PR #757.

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 also open this branch up, and just overwrite the file with a saved copy of the unmodified file, so that this PR will show only those changes which address it's purpose. Thank you!!! That will help keep each fix in its own PR and separate commits. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I get it now. I'm sorry for the messy details. I will fix it today and push again.
Thank you and happy new month 🥳

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Once again, I sincerely apologise for the back on forth on a minimal change like this. I have removed changes that were meant for PR #757 as @TildaDares mentioned and the current commit now carries only the change required for this PR.
Please let me know if this works or if you need me to do a git reset --hard instead.
cc @jywarren

Copy link
Member

Choose a reason for hiding this comment

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

No worries! Part of the learning process! Let me see now that I merged #757

@@ -50,7 +50,7 @@ module.exports = function initTables(_module, wysiwyg) {
var builder = '<div class="form-inline form-group ple-table-popover" style="width:400px;">';
builder += '<a id="decRows" class="btn btn-sm btn-outline-secondary"><i class="fa fa-minus"></i></a> <span id="tableRows">4</span> <a id="incRows" class="btn btn-sm btn-outline-secondary"><i class="fa fa-plus"></i></a>';
builder += ' x ';
builder += '<a id="decCols" class="btn btn-sm btn-outline-secondaryt"><i class="fa fa-minus"></i></a> <span id="tableCols">3</span> <a id="incCols" class="btn btn-sm btn-outline-secondary"><i class="fa fa-plus"></i></a>';
builder += '<a id="decCols" class="btn btn-sm btn-outline-secondary"><i class="fa fa-minus"></i></a> <span id="tableCols">3</span> <a id="incCols" class="btn btn-sm btn-outline-secondary"><i class="fa fa-plus"></i></a>';
Copy link
Member

Choose a reason for hiding this comment

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

Great working on spotting this!

@@ -22475,7 +22475,7 @@ module.exports = function(textarea, _editor, _module) {
);
$(".wk-commands, .wk-switchboard").addClass("btn-group");
$(".wk-commands button, .wk-switchboard button").addClass(
"btn btn-light"
"btn btn-outline-secondary"
Copy link
Member

Choose a reason for hiding this comment

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

@NARUDESIGNS What is this change for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@TildaDares the change keeps happening automatically every time I make changes to src/modules/PublicLab.RichTextModule.Table.js

Copy link
Member

Choose a reason for hiding this comment

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

I've left a comment that explains this in #757

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I'm just responding now. I followed your instruction in #757 and I've made all necessary changes as you required.
@TildaDares

});
$(document).on('click', '#incCols', function() {
$("#tableCols").text( Number($("#tableCols").text()) + 1 );
});
$(document).on('click', '#decCols', function() {
$("#tableCols").text( Number($("#tableCols").text()) - 1 );
const numOfCols = Number($("#tableCols").text());
if (numOfCols > 1) $("#tableCols").text( numOfCols - 1 );
Copy link
Member

Choose a reason for hiding this comment

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

Hi @NARUDESIGNS, this file still has those changes.

Copy link
Collaborator Author

@NARUDESIGNS NARUDESIGNS Dec 6, 2021

Choose a reason for hiding this comment

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

Oops! My bad.
I should push the dist/PublicLab.Editor.js file as well.
I think those changes are fine in my local env.

Thank you @TildaDares

Copy link
Member

@TildaDares TildaDares left a comment

Choose a reason for hiding this comment

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

Thank you @NARUDESIGNS. Amazing work!!

@jywarren
Copy link
Member

jywarren commented Dec 7, 2021

Resolved the conflicts (it did it automatically) and approved the test run, so lets see! 🤞

@jywarren
Copy link
Member

jywarren commented Dec 7, 2021

This should be fine as it's a CSS change. But occasionally tests depend on CSS classes, so good to check!

@NARUDESIGNS
Copy link
Collaborator Author

This should be fine as it's a CSS change. But occasionally tests depend on CSS classes, so good to check!

Tests bother me a lot and I think it's because I know it's tied to most of my task list and so if it doesn't get resolved on time then my tasks will not get started soon. Although some of the tasks are CSS changes, unlike the ones that involve clicks and button events.

@jywarren jywarren merged commit f8e1edb into publiclab:main Dec 7, 2021
@jywarren
Copy link
Member

jywarren commented Dec 7, 2021

Looks good, nice to see some initial PRs merged, huh?

@jywarren
Copy link
Member

jywarren commented Dec 7, 2021

Don't worry, you'll figure the tests out and it'll feel great!

@NARUDESIGNS
Copy link
Collaborator Author

Looks good, nice to see some initial PRs merged, huh?

Yh lol. Seeing your PR get merged and hopefully, deployed to production feels good 😊😊😊

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.

3 participants