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

upgrade Bootstrap to 4 #566

Closed
wants to merge 15 commits into from
Closed

Conversation

keshav234156
Copy link
Member

#508
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt jasmine
  • code 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/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

@gitpod-io
Copy link

gitpod-io bot commented Jul 18, 2020

@keshav234156
Copy link
Member Author

keshav234156 commented Jul 18, 2020

@jywarren @sagarpreet-chadha @NitinBhasneria @Shreyaa-s @Shulammite-Aso Please review. We can leave the passing of jest-puppeteer test as of now. The test are correct and are passing locally but it look like travis run with --runInBand to fasten te process which fails the test.locally it can be simulated by node ./node_modules/.bin/jest --runInBand . I have also added changes to Gruntfile.js(made it as it was prior to #519 ) and added it to eslintignore as process were not running related to grunt because of it.

@keshav234156
Copy link
Member Author

Also, I have made this new PR (previous PR #561) because my previous forked repo was having too much problem with test and all branches were messed up (I don't why 😕 ?).

Copy link
Contributor

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

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

Looks great 💯

@sagarpreet-chadha
Copy link
Contributor

@keshav234156 , did you find any way we can remove this param from travis so that the tests can pass? Thanks!

@keshav234156
Copy link
Member Author

@sagarpreet-chadha Not yet!!

@cypherean
Copy link
Contributor

I can replicate it locally but why is it triggered on selected PRs? And persists even on restarting travis? Do you have any idea why is it so @keshav234156?

@jywarren
Copy link
Member

If you get really stuck here, perhaps you might reach out to icarito or alaxalves! Thanks for working hard on this, and if at all possible I recommend following conventions established by Travis and Jest, and linking to them so folks can follow along. Thank you!

@keshav234156
Copy link
Member Author

keshav234156 commented Jul 28, 2020

@Shreyaa-s Can you please have a look at these tests as well !!

@cypherean
Copy link
Contributor

cypherean commented Jul 28, 2020

I tried but for this one the last line, i.e. await page.click('.woofmark-mode-wysiwyg'); isn't working. The mode doesn't change. The safeguard added for that in the starting of test works. But in this case the starting of the text gets selected and is changed to bold/italic that's why it couldn't find the string specified since we format the existing selected text, the test only fails for whichever test comes second. There are a few options we could try:

  1. Converting to md and checking for * or _.
  2. Adding await page.evaluate(() => document.querySelector('.wk-wysiwyg').textContent += ' '); but this makes the rest of the text loose all formatting, like this:
    italic
  3. backspace the selected text.
    I wasn't able to figure out why the change mode button didn't work, if anyone can that would be great help. And if we can implement anyone one of these, which one should it be?

@jywarren
Copy link
Member

jywarren commented Aug 4, 2020

Hi @keshav234156 just wondering if you had a chance to try one of @shreyaa-sharmaa's ideas above? Did any of them work?

@keshav234156
Copy link
Member Author

keshav234156 commented Aug 6, 2020

fixed test for bold and italic by backspace. Now need to find a fix for center text.

@keshav234156
Copy link
Member Author

fixed center test as well!!
@jywarren Please review.

@keshav234156 keshav234156 reopened this Aug 7, 2020
@gitpod-io
Copy link

gitpod-io bot commented Aug 7, 2020

Copy link
Contributor

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

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

Good to go after removal of screenshot commands 🎉

@pydevsg pydevsg self-requested a review August 8, 2020 08:24
@keshav234156
Copy link
Member Author

@jywarren Ok doing it now!

@gitpod-io
Copy link

gitpod-io bot commented Aug 17, 2020

@gitpod-io
Copy link

gitpod-io bot commented Aug 18, 2020

@gitpod-io
Copy link

gitpod-io bot commented Aug 18, 2020

@keshav234156
Copy link
Member Author

@jywarren Can you please see why test are not starting

@gitpod-io
Copy link

gitpod-io bot commented Aug 18, 2020

@jywarren
Copy link
Member

Hi, @keshav234156 - one thing that can happen sometimes is that it's easier to just rebase the whole thing cleanly to get Travis to restart. It looks like there's something odd going on here even with GitPod leaving so many comments. Can you try a clean rebase?


require('matchdep').filterDev('grunt-*').forEach(grunt.loadNpmTasks);
grunt.loadNpmTasks('grunt-browserify');
Copy link
Member

Choose a reason for hiding this comment

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

Uh oh - were these commits supposed to make it into here? Just checking!

Copy link
Member

Choose a reason for hiding this comment

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

Ah, i see here! all good then! #566 (comment)

@jywarren
Copy link
Member

Hi! Almost there -- i see a few small issues!

I see a small issue with the "Help" icon here:

image

And the Use current location button needs an outline, as do a few other buttons. I think they work better with outlines otherwise they are hard to see as buttons, you know?

image

image

The history button too:

image

@keshav234156 I think this will be all good once you can make these tweaks, thank you so much!!!

Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

Just noting that we need a few Bootstrap CSS fixes. Thanks!!!

@jywarren
Copy link
Member

Hi @keshav234156 i hope you're well! Do you think you'll have any time to wrap this issue up, or do you think it'd be best if someone else took a look at it? Thank you!!!

@Sagarpreet Sagarpreet mentioned this pull request Oct 7, 2020
14 tasks
@Sagarpreet
Copy link
Contributor

Moved here: #636
Thanks @keshav234156 💯

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.

8 participants