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

Migrate Postwoman to Nuxt.js (full Vue and SCSS support) #44

Merged
merged 4 commits into from
Aug 24, 2019
Merged

Migrate Postwoman to Nuxt.js (full Vue and SCSS support) #44

merged 4 commits into from
Aug 24, 2019

Conversation

NBTX
Copy link
Contributor

@NBTX NBTX commented Aug 24, 2019

I have migrated Postwoman to Nuxt.js configured with static site mode.
In this PR, I've also updated the README to reflect these changes.

This PR solves #31.
I did not use https://github.com/chrisvfritz/vue-enterprise-boilerplate, as @diego81b proposed, because it would - as he said - need to be appropriately resized. Nuxt is, at least in my opinion, better in this respect as it is far more modular and allows us to just pick and choose modules we want.

Key changes are the styles.css file: => /assets/css/styles.scss (you'll also see fonts.scss that I created specifically for adding, importing and declaring font faces.)

All of the PWA code has been moved into a standalone file => /assets/js/pwa.js (which is used in /layouts/default.vue.)
The error page is now in /layouts/error.vue

The main application page is in /pages/index.vue - except for the header and footer, which is in /layouts/default.vue so it'll be added to every page.

Finally, the main application code has been moved from script.js to /pages/index.vue (I didn't really want to mess with the application JS itself all in one go.)


Quick footnote: Nuxt does have support for tailwind CSS but your CSS code is tidy and when it comes to design you really seem to know what you're doing! 😃
So I did not include the Tailwind module because I personally don't think it's necessary, however if you decide you would prefer to have the module, it's easy enough to add.

@NBTX NBTX changed the title Migrate Postwoman to Nuxt.js (full Vue and SCSS) Migrate Postwoman to Nuxt.js (full Vue and SCSS support) Aug 24, 2019
@TravisBuddy
Copy link

Hey @NBTX,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 6bc4c680-c61d-11e9-8aae-f1e5ed08699b

@TravisBuddy
Copy link

Hey @NBTX,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 6fbd29d0-c61d-11e9-8aae-f1e5ed08699b

@TravisBuddy
Copy link

Hey @NBTX,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: ffb4d370-c61e-11e9-8aae-f1e5ed08699b

@TravisBuddy
Copy link

Hey @NBTX,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 02dbdd00-c61f-11e9-8aae-f1e5ed08699b

@liyasthomas liyasthomas self-requested a review August 24, 2019 03:32
@NBTX
Copy link
Contributor Author

NBTX commented Aug 24, 2019

Also, if you want to test this, you can just clone my fork and follow the instructions in the README.
Link for convenience: https://github.com/NBTX/postwoman-vue/

@liyasthomas liyasthomas self-assigned this Aug 24, 2019
@liyasthomas liyasthomas added feature New feature or request future Scheduled to near future need testing Needs to be tested before merging onto production labels Aug 24, 2019
@liyasthomas liyasthomas added this to the v1.0 Stable release milestone Aug 24, 2019
@liyasthomas
Copy link
Member

I hope this probably solves #37 #32 #31

@NBTX
Copy link
Contributor Author

NBTX commented Aug 24, 2019

Can confirm this fixes #37.

image

@liyasthomas
Copy link
Member

I'll merge this after some testing

@AndrewBastin
Copy link
Member

Umm, this will shit up all the existing PRs...

But this is something we need bad....

@TravisBuddy
Copy link

Hey @NBTX,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 38fc0220-c66f-11e9-be9f-2924d72427a9

@TravisBuddy
Copy link

Hey @NBTX,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 3d8dbf40-c66f-11e9-be9f-2924d72427a9

@liyasthomas
Copy link
Member

@NBTX lets do it. But i need your help on fixing #33 since @AndrewBastin said this will probably break his PR for "Code highlight on response body". Please check out his commit: AndrewBastin@73b5c6a and consider an alternative for it.

@liyasthomas liyasthomas merged commit 43c2bdf into hoppscotch:master Aug 24, 2019
@AndrewBastin
Copy link
Member

@NBTX lets do it. But i need your help on fixing #33 since @AndrewBastin said this will probably break his PR for "Code highlight on response body". Please check out his commit: AndrewBastin@73b5c6a and consider an alternative for it.

Hey @liyasthomas, no probs, I will rework the commit for the new system, as I said, the current system is really hacky.... Not a big fan of it, it kinda comes really close to XSS exploitability than I like...

@liyasthomas
Copy link
Member

@NBTX hosting onto GitHub pages is broken/missing
Refer here: https://nuxtjs.org/faq/github-pages/ and fix hosting as ASAP

@liyasthomas
Copy link
Member

liyasthomas commented Aug 24, 2019

I updated .travis.yml and build pushed to gh-pages branch but GitHub pages deployment keeps on loading spinner. I'm not with my PC right now to check what went wrong. Please do check it and inform me. Travis CI job: https://travis-ci.org/liyasthomas/postwoman

@larouxn
Copy link
Contributor

larouxn commented Aug 24, 2019

Would it be worth considering temporarily reverting this PR and aae579c until Nuxt deploys have been fully figured out? Considering the app is down at the moment and all.

@AndrewBastin
Copy link
Member

@larouxn yeah, I think so too... we should really have been working on this in a separate branch as this aint stable yet...

So, I can still work on improvements on the new system, while peeps can still use the app and these issues can be resolved

@larouxn
Copy link
Contributor

larouxn commented Aug 24, 2019

I think this revert PR (#47) should do the trick. 🤞

@larouxn
Copy link
Contributor

larouxn commented Aug 24, 2019

Confirmed. We're back up. 🚀
Screen Shot 2019-08-24 at 23 43 47

@liyasthomas
Copy link
Member

liyasthomas commented Aug 24, 2019

Reverted. Sorry for the inconveniences caused. I checked locally and it worked. That's why I merged. But deployment went faulty. I'll look into it as soon as I can.

@AndrewBastin
Copy link
Member

@liyasthomas @NBTX can we get this merged into a separate branch titled something like 'migrate-nuxt' until we can figure out how to work out this stuff ?

Helps out a ton for working on features...

A small NOTE on Readme for new contributors would also be nice to let them know that we are migrating to Nuxt

@liyasthomas
Copy link
Member

@AndrewBastin Will have seperate branches until everything get fixed.

@diego81b
Copy link

Another stuff to think about is the usage of the pwa plugin shipped with nuxt instead of the actual custom flow.

IMHO relying on localstorage is a little bit risky the same for installation (mobile browser already manage installarion prompt without any manual trick).

Nuxt behind the scene use workbox (if i remember well) that works well for many scenarios (updated version, caching resources, ecc...)

My 2 cents 😉

@liyasthomas
Copy link
Member

I've used workbox on most of my projects. Workbox integrates seamlessly with PWA, service workers and all other PWA technologies. If there's a PWA plugin exclusively for out of the box support, I would like to see it implemented.

@liyasthomas
Copy link
Member

@NBTX
Copy link
Contributor Author

NBTX commented Aug 24, 2019

@diego81b Yes Nuxt uses workbox internally.
@liyasthomas My bad, I didn't check it with gh-pages myself either. I'll work on fixing that and then should I open another PR?

@liyasthomas
Copy link
Member

Of course you should consider making another pulls. All of us would love to see this feature implemented 🔥✌️

@NBTX
Copy link
Contributor Author

NBTX commented Aug 24, 2019

@liyasthomas I've fixed the Travis configuration. I think the problem may have been the missing router base path configuration. (It's automatically generated during npm run build now.)

This time I've tested it - and all appears good 😄
https://nbtx.github.io/postwoman-vue/

@NBTX
Copy link
Contributor Author

NBTX commented Aug 24, 2019

Refer to PR #49 for the changes.
I've also included instructions on how to set up Travis and GitHub pages in .travis.yml.

@sheecegardezi
Copy link

Thanks for this PR, this made it a lot easier for me to understand the flow of the app :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request future Scheduled to near future need testing Needs to be tested before merging onto production
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants