-
Notifications
You must be signed in to change notification settings - Fork 2
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
Contact app mobile #752
base: develop
Are you sure you want to change the base?
Contact app mobile #752
Conversation
…ing the mobile contacts example
- implements grid and images view - scales grid view down to just name and location to better use narrow space - grid / image button group uses images to better use space - some styling work to prevent overflow of top bar UI in common widths
…mobile. - Prep for this to migrate to src/examples/
…contactMobile path
- webpack seemed to want to load the base "contacts" every time any path was requested with "contact" in the name - contactMobile was always being loaded using the contact.js bunble - mobile seems not to have the same issue, so mobileContacts is loading with no issue
- details page is by defaul flex none and width 0 - selecting an option adds a class to show the details - X button added to details header which removes the class and hides the details
…olling of child elements
- favorites button will toggle the record as favorite - edit button enabled to allow editing - save button will properly call to persist changes
- favorite icon is present in grid list view - properly show all persisted contact data in details form
- share SCSS files where between desktop / mobile where unchanged - import desktop SCSS into mobile SCSS file where we need to add defintions - some added classes to properly sync UI between desktop and mobile
… mobile - now unused
- add specificity to a custom class to avoid the need for !important (lesser evil) - move an import to the UI comments rather than the model component where I had accidently placed it
…ance (WIP) - persisting in DB now works for view type and favorites - seems unnessessiary boilerplate, so likely this needs more iteration as I learn the process
- css comment update where I needed to increate specificity on a class def - post-save contact update the record rather than resyncing / rebuilding the entire page
- group grid view listing by isFavorite - remove custom isFavorite render (will not show when the column is the group directive) - remove now unneeded styling for isFavorite cell
@@ -0,0 +1 @@ | |||
@import '../contact/DirectoryPanel'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change to CSS, but I do need it present / loaded.
setDisplayMode(value: 'grid' | 'tiles') { | ||
this.displayMode = value; | ||
|
||
XH.setPref(PERSIST_APP.prefKey, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/xh/toolbox/pull/752/files#r1994240772. This was persisting correctly alone, but if I include the above fix to user favorites and NOT this, then saving a favorite will clobber the save of mode.
@@ -0,0 +1,24 @@ | |||
import {hoistCmp} from '@xh/hoist/core'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of these components are visually exactly the same as the desktop counterpoints - so ideally I share some of all of the execution to avoid repetition. However, the need for DESKTOP versions of components there and MOBILE version here means that this is not possible in my testing - the app would complain of any imports to a mobile app which themselves included imports of desktop items.
className: 'tb-contact-tile__bar', | ||
// This inline style defintion keeps me from needing to fight with | ||
// existing styles when making the mobile button want to center itself | ||
style: {alignItems: 'center'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a fair bit of fighting the styling while working on this.
We define some styles in SCSS files to customer a component or layout. Said component / layout will already have some styling present from existing styles from XH / TB / BP. Then additionally you can define some styles on an element component which are writing direct to the styles tag.
This ultimately meant for me that any styles I applied in an SCSS file were clobbered right off by either an existing style tag or - more often - existing styles with far greater specificity. In this case, using an inline style was of highest import, and so was allowing this align-items style to win over the native styles applied to a mobile button.
@@ -0,0 +1,48 @@ | |||
.tb-mobile-contact-details-panel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is not entirely different from the desktop styles, but for mobile I needed CSS animations. Trying to inhert the common parts was proving troublesome due to the nesting.
const {currentRecord, visible} = model; | ||
|
||
return panel({ | ||
className: `tb-mobile-contact-details-panel${visible ? '--details-visible' : ''}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hide it until needed, since its "off screen".
} | ||
}); | ||
|
||
const contactProfile = hoistCmp.factory({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the definitions below here are another instance where they LOOK the same as the desktop counterparts, but use MOBILE versions of components.
const {readonly, isDirty} = formModel; | ||
|
||
// Pulled from standard details page | ||
// How could the form be "dirty" if it's readonly? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if I am missing something in the isDirty logic?
…rent - Shared components can now live in the root - name updates to mobile UI and model to align with the desktop names
// Shared from the desktop version | ||
import {PERSIST_APP} from '../AppModel'; | ||
|
||
import ContactDetailsModel from './details/DetailsPanelModel'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Importing this under a different name to the class is confusing - let's use the same name in both the definition and its usages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must have been a missed update. Originally the files had bespoke names in the mobile app, but in a second pass I unified them with the desktop naming. Must have missed this change to the name reference. Ill update.
const {currentRecord, visible} = model; | ||
|
||
return panel({ | ||
className: `tb-mobile-contact-details-panel${visible ? '--details-visible' : ''}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use a mobile navigator to handle drilldowns. This will allow us to:
- Make the details its own page, meaning you don't need to handle visibility / transitions
- Route to specific contacts by name
- Support gesture navigation on device, etc
import {panel} from '@xh/hoist/mobile/cmp/panel'; | ||
import {Icon} from '@xh/hoist/icon'; | ||
import {AppModel} from '../AppModel'; | ||
import {contactsPage} from './DirectoryPanel'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've said elsewhere, but renaming on import is confusing - lets keep import names, file names and class names consistent throughout
…ation flow - add link to desktop contacts to load mobile contacts
- this allows contacts to be shared between distinct child pages (directory and details) - migreates edit / favorite saves to app model - detail page uses a plain object rather than a grid record
@@ -14,6 +15,13 @@ export const AppComponent = hoistCmp({ | |||
return panel({ | |||
tbar: appBar({ | |||
icon: Icon.contact({size: '2x', prefix: 'fal'}), | |||
leftItems: [ | |||
a({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New link to let users access mobile contacts from the existing toolbox UI
themeAppOption | ||
} from '@xh/hoist/mobile/cmp/appOption'; | ||
|
||
export default class AppModel extends BaseAppModel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
App model, as the root model for mobile contacts, now does a. lot more management of its children - including storing the fetched contact data and instantiating the models for the child views.
runInAction(() => { | ||
this.tagList = uniq(this.contacts.flatMap(it => it.tags ?? [])).sort() as string[]; | ||
}); | ||
this.directoryPanelModel.loadAsync(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
App fetches contacts, then tells the directory to load its grid.
+ Grid records observe changes to AppModel.contacts + Detail panel observes changes to AppModel.contacts
Suggested adjustments to mobile contact app
…hard coded option
…ly in mobile - mobile select cannot "multi select" - need to cast the single result to an array to save - use render to display tags correctly
@@ -2,8 +2,8 @@ import '../Bootstrap'; | |||
|
|||
import {XH} from '@xh/hoist/core'; | |||
import {AppContainer} from '@xh/hoist/desktop/appcontainer'; | |||
import {AppComponent} from '../examples/contact/AppComponent'; | |||
import {AppModel} from '../examples/contact/AppModel'; | |||
import {AppComponent} from '../examples/contact/desktop/AppComponent'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will se this update across this PR.
After some back and forth, Lee suggested that I nest the new mobile
contacts under the parent /examples/contacts, and do the same with desktop
.
This means that some of this PR will be just file path adjustments to accomodate this change.
|
||
export const AppComponent = hoistCmp({ | ||
displayName: 'App', | ||
model: uses(AppModel), | ||
|
||
render() { | ||
const domain = window.location.origin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if there is a 'hoist' way to access the URI.
import {ContactService} from './svc/ContactService'; | ||
import {BaseAppModel} from '../../BaseAppModel'; | ||
|
||
export const PERSIST_APP = {prefKey: 'contactAppState'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As desktop AppModel is no longer the root for all of contacts, I moved this to here, which is common to both versions.
// Mobile select only allows single selections, while desktop allows multiple | ||
// This means we receive an array on the desktop picker, and a single value here. | ||
// Convert single result into an array of one to keep data consistent. | ||
if (data.tags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment above explains this. At some point maybe mobile needs to be enhanced to support multi-select, but that seems out of scope here.
…n SASS file - was originally used for custom animtions, which are no longer used
gradle.properties
Outdated
@@ -3,7 +3,7 @@ xhAppName=Toolbox | |||
xhAppPackage=io.xh.toolbox | |||
xhAppVersion=7.0-SNAPSHOT | |||
|
|||
hoistCoreVersion=29.0-SNAPSHOT | |||
hoistCoreVersion=30.0-SNAPSHOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didnt make this upate. This is what is on develop
https://github.com/xh/toolbox/blob/develop/gradle.properties#L6
When I merged up this appeared. I rebased origin/develop into local/develop, then merged local/develop into this branch. That has never before made a commit list which was not mine.
this.detailsPanelModel = new DetailsPanelModel(this); | ||
} | ||
|
||
async updateContactAsync(id: string, data: PlainObject = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is data
optional here? I don't see why you call this without data
?
If not, I'd recommend we lose the default {}
value, and instead return early if data
is null, to avoid the call for to the server for a no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
As an onboarding exercise, it was suggested that I would build a mobile version of the existing toolbox contacts app. I used this to
I pulled as much of the common parts of the desktop contact app as I could - specifically sharing SCSS definitions where overrides or replacements were not needed, and sharing the contact service, which as an API should behave the same between the versions.
However, sharing implementation code proved messier, and may. not be possible in any way where its complexity would be justified in the saved footprint. Currently if I try to make this "common" component builder step, the code fails to execute with an error that a mobile application is importing desktop components (and vice versa). This will mean that for now, any code updates may need to be done in two separate places, violating a pretty basic DRY principle.
I will provide inline comments to this code where specificity is merited, but a few top level notes here:
https://login.xh.io/oauth/token
), and this appears to stem from it not liking my providedredirect_uri
(in this casehttp://localhost:3000/mobileContacts/
). I worked around this for my work by just disabling outh in my local hoist-core, but we will need to look into this if any new routes are merged/contactsMobile
(plural contactS) rather than/contactMobile
, and webpack was serving me the chunkhash for the standard contact page. It seems the webpack server does some sort of pattern matcher for entry points where it finds the closest entry point matching the provided pathname left-to-right. So for instance, I can load/app
as I would expect, but/apppppppp
would also load the base app entry point./ap
however would not, since it does not make an exact match before the pathname ends. Not critical, just weird and I could not find the cause. Would be good to better understand.Note:
The inline comments on this PR are almost all still valid, despite being noted as "outdated". Lee suggested reconfguring my folder choices to create a root "contact" folder which can house common / shared files, then within create "desktop" and "mobile" folders for respective, dedicated code. This was a full refactor of file structure, which broke the previous comments - hense them showing as outdated.