ADDS: Feature to decenter text in wysiwyg and markdown mode#521
ADDS: Feature to decenter text in wysiwyg and markdown mode#521jywarren merged 7 commits intopubliclab:mainfrom
Conversation
|
The local tests are running fine but ci tests are still on hold, any idea what might be wrong? @jywarren @Shulammite-Aso @keshav234156 @NitinBhasneria ? |
|
I have made a PR in this regard |
|
#520 ? Let's wait for it to get merged then. Thanks! |
|
@jywarren @emilyashley @VladimirMikulic @keshav234156 @NitinBhasneria @Shulammite-Aso please review. |
VladimirMikulic
left a comment
There was a problem hiding this comment.
If you could fix formatting and have a consistent style, that would be great...
|
code looks great @Shreyaa-s do you mind making a GIF?? |
|
@Shreyaa-s Where are the changes made in? in dist/* folder directly? |
There was a problem hiding this comment.
@Shreyaa-s as @keshav234156 said, you have made changes in dist folder this is not a good practice as dist folder will build every time we run grunt due to which the changes will be lost.
91b66f6 to
eefa201
Compare
|
@NitinBhasneria yes, you are right committing @Shreyaa-s please run build and commit changes in the dist. |
|
Done. Thank you everyone! |
|
Hi @Shreyaa-s, I was reviewing this PR. If we at the start press center button it doesn't do anything. I think it should start writing at the center if no text is selected at the beginning. |
|
Okay @keshav234156 . I'll make another PR for this. |
|
This looks great!!🎉 |
|
Hi all, great collaboration! Thank you for being so constructive and supportive in your feedback on this PR. You can never use enough emoji 😍 🎉 💯 just noting:
I believe @Shulammite-Aso has suggested this (i may be mixed up) as a kind of state management way of managing formatting for this scenario. Would this be at woofmark or in this repo? Can someone help connect the dots to where that discussion was happening? Just want to be sure we stay coordinated. Thanks! |
jywarren
left a comment
There was a problem hiding this comment.
Wow this looks good. @Shreyaa-sharmaa what do you think about writing a test to demonstrate this working? In general this is a best practice to secure functionality against future changes and I think at this point we should all start to do this.
It also makes reviewing much easier as we can read the test to confirm what the code is doing!
I'm not sure if I have come across this discussion before, @Shulammite-Aso would be able to guide us better here. I believe this should be done in this repo. We can add a condition |
|
@Shreyaa-sharmaa I think this is the comment here #490 (comment) maybe some code inside an Thanks🎉 |
|
The test flow I though of was:
Is that right? And feasible? |
|
That sequence for tests sounds GREAT. Thank you!!! 🎉 |
|
Hi! I think we are ready for this to get a Jest test now! After #541 is now complete! |
|
I'm working on that test now. |
1469be3 to
4be3160
Compare
|
Added the test here. I believe it is failing for the same reason as that mentioned here #543 (comment). Local results for the tests: |
|
Travis should pass after merging of #543 |
|
Resyncing! If it doesn't pass, perhaps it may need a rebase. Thanks, and great work! |
|
Hi @Shreyaa-sharmaa -- can you try rebasing this and hopefully it'll pass? Thanks! 🎉 |
33d453f to
b487db7
Compare
b487db7 to
664bf5c
Compare
|
@jywarren this is ready to be merged. |
sagarpreet-chadha
left a comment
There was a problem hiding this comment.
excellent!
Just some changes required, also it is good practice to use const instead of var.
Also test is written beautifully! 🎉
|
Made the required changes. Please have a look @sagarpreet-chadha. Thanks! 😄 |
I used |
sagarpreet-chadha
left a comment
There was a problem hiding this comment.
Awesome, looks good 👍
Yes we can use const going forward
|
Updating branch to then merge! Thanks! |
jywarren
left a comment
There was a problem hiding this comment.
Very nice, and love the tests. Thank you!




FIXES: Decentering of a centered text #489
Enables decentering in both modes
Re-aligning center aligned text left aligns it
Selecting center aligned text as a substring and re-aligning the string center aligns the complete string
tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with
grunt jasminecode is in uniquely-named feature branch and has no merge conflicts
PR is descriptively titled
PR body includes
fixes #0000-style reference to original issue #ask
@publiclab/reviewersfor help, in a comment below