Closed yivo closed 6 years ago
Can you please get Peatio running and review this task? I am sure you will some questions. Please ping me via GitHub mentions here. I think the strategy Create new Rails app and include existing assets.
is the fastest.
@dinesh-skyach ping
@mkuvshynnikov ping
@yivo @mkuvshynnikov Please could you let me know which trading page is being referenced here. Also here we need to separate the trading page to new rails app and then fetch the data from API. Is this correct?
@dinesh-skyach The page we refer to localed at path /markets/btcusd
(black one). Yes, I think you can create repo with new Rails app and move trading UI there and use API to communicate.
@yivo Thanks
@yivo since you said above Only UI (no server-side operations) in new rails app using middleman then how will we handle the authentication process. As trading page has user information along with other data which is required on trading page.
@dinesh-skyach Hello. Don't use middleman. Create simple Rails app with Sprockets. Auth is possible via cookies (we will put UI on the same domain so browser will send cookies which include session).
@yivo thanks. I have been working using middleman as per your comment on suggestions but no worries will remove middleman and work with the rails app.
@dinesh-skyach I don't mind to use middleman. If this makes your task faster – you can choose. I planned to use Rails so we can just migrate existing UI without significant changes.
@yivo sure thanks
@yivo hello. I have been working on this and it's almost done. Form submissons for buy and sell and some other stuff's are still pending. Just wanted to update you regarding this. In Addition I am using middleman to separate the UI.
@dinesh-skyach Could you please submit WIP (work in progress) PR?
@yivo As this is a new App so which repo should I commit the code and how will I send you the PR?
@dinesh-skyach Hello. We are waiting for the PR for rubykube/peatio to remove trading page UI and repository with extracted trading UI. You can create the repo using your GitHub account and share the link. When you will finish I will checkout git repo to rubykube org.
Hi @yivo , I will send that in some time as there are a few session concerns, just trying to fix a few more things.
Hi @yivo , I have created the PR for this Issue. Also committed the new trading page UI code to new repo. Here is the link https://github.com/skyach/petio-trade.
I have used the same controller methods to fetch the data as json format and displayed the data in new trading app.
In Addition I am still thinking about the best solution for POST requests. As currently I have skipped the form authentication token for these methods. I am still looking into this.
@dinesh-skyach Is there any solution to keep peatio-trade-ui free of Rails controllers?
We can solve authentication issue by adding OAuth sign in and interacting with API using JWT. Can you check what API calls markets page is using and what are missing? Maybe we can move some code from private/controllers to Grape API.
@yivo , I think if we end Api request through JS, then something is achievable, but let me try. If that works well, then I will think of Oauth with JWT.
@yivo , Even if I remove the private/markets controller , I have to move the views to application/views folder and call APi though JS only, the application controller still remain there as role of Application controller does not go away in MVC, since rails has to follow strict MVC, so, it also dissolves the purpose of removing markets controller. Your thoughts?
Another option is that, I can move the html files to public and call Apis with JS, but there will be no point of using rails in that case. Please let me know your inputs, I am working on Saturdays as well.
Hi @yivo , Any inputs on this?
@dinesh-skyach I see the problem.
Let's do so:
This is the most fastest way. And it is less coding I think. You need to:
gem 'rails', '4.2.10'
). Can you generate Rails app using my command? I have prepared good one: rails new peatio-trading ----skip-yarn --skip-action-mailer --skip-active-record --skip-action-cable --skip-spring --skip-turbolinks --skip-test --skip-system-test
.@dinesh-skyach Do you understand this? The trick is that two app will live on the same origin so browser will send cookies.
@dinesh-skyach Ping me at eahome00@gmail.com if you would like to make a call for this. My TZ is UTC+2.
Hi @yivo , All is clear, just a confusion about point 11, where you said Angular App. But as application is not using angular till now, and neither you said the same from point 1 to 10. Should I consider using angular in this app or as its currently in rails is good?
@yivo , I am just adding to my gtalk.
@dinesh-skyach Oh, yes. It doesn't use angular. You should extract all assets required for markets to work (market.js.coffee, market.scss and maybe other).
Hi @yivo , I have strictly followed your steps. Redirect all request of trading UI to Private::MarketsController. Please let me know if we need move that code to JWT. I have also added CORS to gemfile, since on local we have to handle these on different ports locally. But on production it may not be required. Please find the trading UI code at the following repository:- https://github.com/skyach/peatio-trading-ui
Please let me know if you still need improvements.
@yivo , One more question I had, how are you going to link there 2 apps in development environment. For linking, I think, we ned to define the linking url in YML, which devs can change according to the domains, as on local it will behave differently than production.
@dinesh-skyach Hello. Thank you for the good job. Can you please refactor to use Yarn for this folder? https://github.com/skyach/peatio-trading-ui/tree/master/vendor You can see the example here in Peatio repository. Can you please also cleanup peatio-trading-ui from unneeded locales, JS and CSS files?
@dinesh-skyach I think in dev environment Peatio will require link to trading app via ENV variable. In production it will use route.
Can you move this variable to peatio-trading-ui? https://github.com/rubykube/peatio/blob/devel/lib/generators/config/templates/application.yml.erb#L114
Can you also add variable MARKETS_URL
?
Hi @yivo I have made the above changes.
In Addition I have used yarn and as we are using Rails 5 for the new Trading App, so now the yarn modules are added under node_modules folder.
Also I have removed the unneeded JS and CSS files. And currently working on removing the unneeded Locales.
@dinesh-skyach Cool. We are working now on patch which should simplify locales and reduce it's number. I think it will help you a lot after merge.
Hi @yivo, I will wait for the patch and then will remove the unneeded locales from Trading UI App.
Meanwhile I have made the fixes for the Trading UI App. Also for the development environment we need to add the CORS headers for AJAX requests, so I have added the CORS headers for the development enviornment but this will not be required for Production as the domain will be same.
Let me know if you have some other ways to handle this.
@yivo Any updates on the patch?
Closed via #449.
The probably strategies:
NOTES: