Skip to content

Conversation

@permission-error
Copy link
Contributor

@permission-error permission-error commented Oct 26, 2023

Tags @wbrunette @elmps2018

Fixes part of odk-x/tool-suite-X#149

I've spent the past three days working on this pull request. The odkData.js functions are quite many, and as a result, this PR is likely to be quite slow. To make the review process more manageable, I've decided to document only half of the functions for now.

In addition to this PR, I have two more similar PRs in progress, each with some issues that need addressing. Therefore, I plan to focus on addressing the feedback and issues for these three PRs first. Once those are in good shape, I'll continue documenting the remaining functions in odkData.js.

Copy link
Contributor

@elmps2018 elmps2018 left a comment

Choose a reason for hiding this comment

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

Thanks for working on these and good idea to divide up!

Copy link
Contributor

@elmps2018 elmps2018 left a comment

Choose a reason for hiding this comment

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

Thanks for working on these! @wbrunette should review for more technical details as well.


#. obtaining information about the runtime environment (e.g., Android OS version, etc.)
#. obtaining information about the currently-selected locale.
#. obtain the active user.
Copy link
Contributor

Choose a reason for hiding this comment

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

obtaining here and next for consistent tense.


- :file:`system/tables/js/odkTables.js`

This wrapper object mostly invokes `odkCommon` to perform its actions, but does call the `odkTablesIf` injected interface's one method to load the list view portion of the split-screen detail-with-list-view layout.
Copy link
Member

Choose a reason for hiding this comment

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

I would remove line 193. Simply state that odkTables.js is for controlling the tables app.


This wrapper object mostly invokes `odkCommon` to perform its actions, but does call the `odkTablesIf` injected interface's one method to load the list view portion of the split-screen detail-with-list-view layout.

The Survey interface is invoked within the Javascript that implements the survey presentation and navigation logic and should not be directly called by form designers.
Copy link
Member

Choose a reason for hiding this comment

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

You list the file location of the other 4 java script files, so it seems like to match, you should list the location like others for consistency.


:file:`system/tables/js/odkTables.js`

It provides methods to open Tables generic web pages and list and detail views. These are generally just wrappers for calls to `odkCommon` to invoke the intents for those views.
Copy link
Member

Choose a reason for hiding this comment

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

This is a better summary of tables interface. I suggest just having it one place.

@wbrunette
Copy link
Member

wbrunette commented Jan 2, 2024

It would be better to be separated into separate pages per interface.

@elmps2018
Copy link
Contributor

@permission-error good progress on dividing out the data page! @wbrunette were there other pages needing splitting?

@permission-error
Copy link
Contributor Author

@permission-error good progress on dividing out the data page! @wbrunette were there other pages needing splitting?

@elmps2018 I think I need to split the three of them in the 3 pull requests. I just started with this, am working on others as well

@elmps2018
Copy link
Contributor

@permission-error great, please let us know when you are done with all of them so we can review them together.

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.

3 participants