create page for thread and multicolumn extract #432
Conversation
MilanBarande
changed the title
MilanBarande
force-pushed
the
thread-multicolumn-extraction-page
branch
from
eee2f06
to
e999ec2
Compare
|
|
||
| .export-language-dropdown { | ||
| margin-top: 20px; | ||
| position: absolute; |
There was a problem hiding this comment.
I think because there is no position on any of the parents, this position will be absolute to the viewport, which might not be what you're looking for in case of mobile. Please verify.
There was a problem hiding this comment.
This CSS is not from me actually, but I tried removing this position:absolute and the dropdown doesn't appear at all. As there is no admin on mobile is it really worth it to change this ?
There was a problem hiding this comment.
Oh yea! There is no mobile admin section! Okay, then keep this. Good call.
| this.props.router.setRouteLeaveHook(this.props.route, null); | ||
| } | ||
|
|
||
| handleExportLocaleChange = (locale) => { |
There was a problem hiding this comment.
I would bind these functions in the constructor of this class.
eg.
this.handleExportLocaleChange = this.handleExportLocaleChange.bind(this);
https://medium.com/shoutem/react-to-bind-or-not-to-bind-7bf58327e22a
There was a problem hiding this comment.
But the point of using an arrow function here is to avoid having to bind the function
There was a problem hiding this comment.
It's a choice, I suspect. Arrow functions are binding your method which you are passing down to child components. But, binding them in constructor is just a tiny bit more efficient for the JS interpreter to translate.
There was a problem hiding this comment.
Okay I didn't know. I think we have been using arrow functions all over the project
There was a problem hiding this comment.
So I made the modifications. But I just found this facebook/react#9851
apparently it's pretty much the same thing
There was a problem hiding this comment.
@ayyazdaniaryan class properties are transformed with babel (click on the Try button in the example https://babeljs.io/docs/plugins/transform-class-properties/) to an efficient syntax. You can have perf issue with binding in the render method yes because it will create new functions, but class properties are completely different here. We prefer the class properties syntax in the project because of flow.
There was a problem hiding this comment.
@vincentfretin cool! I wasn't aware. Thank you for this update! @MilanBarande good find. TIL.
| this.setState({ exportLocale: locale }); | ||
| }; | ||
|
|
||
| handleTranslationChange = (shouldTranslate) => { |
| /> | ||
| )} | ||
| {!isNaN(currentStep) && ( | ||
| <Navbar currentStep={currentStep} totalSteps={1} phaseIdentifier="multiColumn" /> |
There was a problem hiding this comment.
Why show the Navbar when the admin currently only has 1 step? If it's for the future, I would put another check,
const totalSteps = 1;
!isNan(currentStep) && totalSteps > 1 && ...
In future, if totalSteps increases, the code will be rendered, otherwise the navbar is left out.
There was a problem hiding this comment.
Unless you absolutely want a navbar to be present even in the case of 1 step
There was a problem hiding this comment.
At first I hadn't put it but I thought it made it clear that there is only one step of admin to deal with at the moment (1/1) and not that others are available but not displaying for any sort of reason (bug).
There was a problem hiding this comment.
Ask to the Fred but I remember that he prefer when the navbar id hidden when there is 1 step
| this.props.router.setRouteLeaveHook(this.props.route, null); | ||
| } | ||
|
|
||
| handleExportLocaleChange = (locale) => { |
There was a problem hiding this comment.
Bind these methods as well
There was a problem hiding this comment.
Arrow functions and class properties like this are automatically binded to this @ayyazdaniaryan
There was a problem hiding this comment.
Since we're using Flow, we should use arrow function?
| /> | ||
| )} | ||
| {!isNaN(currentStep) && ( | ||
| <Navbar currentStep={currentStep} totalSteps={1} phaseIdentifier="thread" /> |
There was a problem hiding this comment.
Same comment as above...
| @@ -1286,6 +1288,8 @@ const Translations = { | |||
| export: { | |||
| defaultAnnotation: "You can export all of the data by clicking on the export button", | |||
| surveyAnnotation: "You can export all the survey module data by clicking on the Export button", | |||
| threadAnnotation: "You can export all the thread module data by clicking on the Export butotn", | |||
| multicolumnAnnotation: "You can export all the multicolumn data by clicking on the Export button", | |||
There was a problem hiding this comment.
multi-column or Multi Column as there is no word like this in English.
| import { get } from '../utils/routeMap'; | ||
| import ExportSection from '../components/administration/exportSection'; | ||
| import Navbar from '../components/administration/navbar'; | ||
| import DiscussionPreferenceLanguage from '../graphql/DiscussionPreferenceLanguage.graphql'; |
There was a problem hiding this comment.
Rename in DiscussionPreferenceLanguageQuery for more clarity?
| import { get } from '../utils/routeMap'; | ||
| import ExportSection from '../components/administration/exportSection'; | ||
| import Navbar from '../components/administration/navbar'; | ||
| import DiscussionPreferenceLanguage from '../graphql/DiscussionPreferenceLanguage.graphql'; |
There was a problem hiding this comment.
Rename in DiscussionPreferenceLanguageQuery for more clarity?
No description provided.