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

Replaced Mode button cut off in Updated Label Screen #831

Closed
sebastianbarry opened this issue Nov 4, 2022 · 67 comments
Closed

Replaced Mode button cut off in Updated Label Screen #831

sebastianbarry opened this issue Nov 4, 2022 · 67 comments
Assignees
Labels

Comments

@sebastianbarry
Copy link

sebastianbarry commented Nov 4, 2022

The third "Replaced Mode" button (that displays IF the app is on a program AND the trip's mode is set to "e-bike") gets cut off with the new Updated Label Screen:

iPhone SE Motorola 1 Motorola 2 Motorola 3
Simulator Screen Shot - iPhone SE (2nd generation) - 2022-11-04 at 11 22 12 image2 image1 image4
@sebastianbarry
Copy link
Author

sebastianbarry commented Nov 4, 2022

According to ionic v1 source code, there is a custom height option and a dynamic height option for the collection-repeat HTML element: https://github.com/ionic-team/ionic-v1/blob/master/js/angular/directive/collectionRepeat.js#L268

I found out where the height of the trip-items were hardcoded to 330px here: https://github.com/e-mission/e-mission-phone/blob/19f9a17d6b11b1f58198e6a71099de8caa0b6278/www/css/main.diary.css#L211

.diary-card {
  width: 100%;
  height: 330px;
  margin: 0;
  border: 2px solid rgba(0, 136, 206, 0.2);
  box-sizing: border-box;
  border-radius: 30px;
  overflow: hidden;
  box-shadow: 0 3px 6px rgba(0,0,0,0.16), 0 3px 6px rgba(0,0,0,0.23);
}

and here: https://github.com/e-mission/e-mission-phone/blob/19f9a17d6b11b1f58198e6a71099de8caa0b6278/www/css/main.diary.css#L70

/* ----------- iPhone 6+ ----------- */

/* Portrait and Landscape */
@media only screen 
  and (min-device-width: 414px) 
  and (max-device-width: 736px) 
  and (-webkit-min-device-pixel-ratio: 3) { 
    .angular-leaflet-map { height: 150px; width: 330px; }
}

This hardcoded px value + the height of the start-time-tag and the height of the stop-time-tag is where the hardcoded value of 391px is coming from, as we ran into in #823 (comment) the issue regarding calculating the height of each of these trip items

WHAT DID I TRY? (3 tests)

If we change the values found in .diary-card css (for example to 400px), this is what it looks like:

So the height is big enough to fit the replaced-mode button like this, but the map does not stretch to display.


If we edit the HTML in <colleciton-repeat> to add the expression for a dynamic value (max-height), we get:

<div collection-repeat="trip in data.displayTrips" item-height="max-height">

So all the trips get displayed over each other and the height does not appear to be displaying dynamically between them; they all appear to still be the original 330px or whatever it may be.


If we edit the HTML in <colleciton-repeat> to add the expression for a constant pixel value (400px), we get:

<div collection-repeat="trip in data.displayTrips" item-height="400px">

So we can see that it extends the height of the div around the trip item, but does not extend the height of the trip item because it is still hard-coded to 330px in the css.


Diary card css change collection-repeat item-height with expression item-height hardcoded
Simulator Screen Shot - iPhone SE (2nd generation) - 2022-11-04 at 12 28 07 Simulator Screen Shot - iPhone SE (2nd generation) - 2022-11-04 at 12 49 10 Simulator Screen Shot - iPhone SE (2nd generation) - 2022-11-04 at 12 53 09

@sebastianbarry
Copy link
Author

@shankari I am going to step away for a bit today to run some errands but I will come back today by around 4pm MST

@shankari
Copy link
Contributor

shankari commented Nov 6, 2022

@sebastianbarry when you are planning to come back to this?

  • it seems like changing the diary-card is the right solution overall
    • you just need to change the map height as well so that it fills the card
  • given that you are not setting the item height for the collection-repeat, I think it uses the height of the first element.
    • In that case you need to test carefully to ensure that it works with multiple different cases - e.g. first element has replaced mode, first element does not have replaced mode...

Please list out the full set of use cases in the testing done.

@sebastianbarry
Copy link
Author

I was able to figure out how to dynamically change the item-height!

By using a JS function in the HTML for collection-repeat, we can dynamically set the height based on how many inputs the trip contains:

<div collection-repeat="trip in data.displayTrips" item-height="getTripHeight(trip)">
$scope.getTripHeight = function(trip) {
    if(trip.INPUTS[2]) {
      return 460;
    } else {
      return 390;
    }
  }

This results in the height of the colleciton-repeat correctly being set dynamically.

Screen Shot 2022-11-06 at 10 33 44 PM

Additionally, if we remove the hard-coded height in the css for .diary-card, and replace it with a dynamic height, then we see that the trip items are adjusting their height when the button appears:

.diary-card {
  width: 100%;
  height: 80%;
  margin: 0;
  border: 2px solid rgba(0, 136, 206, 0.2);
  box-sizing: border-box;
  border-radius: 30px;
  overflow: hidden;
  box-shadow: 0 3px 6px rgba(0,0,0,0.16), 0 3px 6px rgba(0,0,0,0.23);
}
Screen.Recording.2022-11-06.at.10.38.10.PM.mov

The problem, as can be seen in that video, is that when we click the button it does not dynamically change the height of the given space for the div of collection-repeat.

This space can be thought of as 2 pieces broken up:

  1. The collection-repeat which is the space in the total list allocated for the trip-item
  2. The diary-card css style, which is the rounded rectangle object that sits inside of the collection-repeat space

I have set the colleciton-repeat part to be dynamic, HOWEVER - if we change the mode of a trip to be "e-bike" AT ALL while it is on our screen, we will need to RELOAD the collection-repeat (either by pressing the refresh button or unloading and reloading through scrolling) in order to see the height dynamically regenerate to be the appropriate height. This is not happening dynamically in front of our face when we click the button. This can be demonstrated in this video:

Screen.Recording.2022-11-06.at.10.31.26.PM.mov

The diary-card piece IS generated dynamically when we click the "e-bike" mode button.


What does this mean?

This means that, we need to get it so that somehow, when you click "e-bike" as the mode for your trip, right at that moment the <collection-repeat> object's height needs to be recalculated, and the CSS needs to dynamically pick the height of the diary-card style for that as well. I am not sure how to do this

One solution I think would solve this, is to have the collection-repeat height be hard-coded to be the height of the tallest possible diary-card with all 3 INPUT options (460px = 30px start time card + 400px diary-card w/ replaced mode button + 30px end time card), and then just allow the CSS to dynamically fill the space if we click the button. This looks like this:

Screen.Recording.2022-11-06.at.10.54.40.PM.mov

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

@sebastianbarry good job! I see what you are saying in general, but the devil is in the details and I am not sure your description matches what I see in the video.

I don't see the collection-repeat height as dynamic in Screen.Recording.2022-11-06.at.10.38.10.PM.mov. It seems like the collection-repeat divs has an height of 461px, even at the beginning of the video when the mode is set to walk. Given that the focus here is on the collection-repeat height calculation, it seems like we should understand exactly when that is computed right now.

@sebastianbarry
Copy link
Author

@sebastianbarry I don't see the collection-repeat height as dynamic in Screen.Recording.2022-11-06.at.10.38.10.PM.mov. It seems like all the collection-repeat divs have an item-height of 460px, even when the mode is set to walk.

I may be mistaken but it looks to me like the heights in collection-repeat are generated dynamically in Screen.Recording.2022-11-06.at.10.38.10.PM.mov. Here's a screenshot of that video with the 2 different heights shown (461px and 391px):
image

Is this not what you are talking about?

Also, my point in Screen.Recording.2022-11-06.at.10.38.10.PM.mov was to demonstrate that the CSS when set with a dynamic height, would change the height of the diary-card when "e-bike" was selected/deselected

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

@sebastianbarry that is what I am talking about. However, the height is already 461px at the beginning of the video when the mode is walk?!

Screen Shot 2022-11-06 at 10 53 57 PM

@sebastianbarry
Copy link
Author

@sebastianbarry that is what I am talking about. However, the height is already 461px at the beginning of the video when the mode is walk?!

Screen Shot 2022-11-06 at 10 53 57 PM

Oh yes I see what you mean, good eye!
I actually had originally had it set to "e-bike" when I first opened the label screen, thus the 461px height. Then I switched it to "walk" and noticed this behavior - so I started my recording software and switched it back to "e-bike"; my mistake but good eye picking up on that!

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

so I am a bit confused. You first say:

This results in the height of the colleciton-repeat correctly being set dynamically.

and then

The problem, as can be seen in that video, is that when we click the button it does not dynamically change the height of the given space for the div of collection-repeat.

So when is it set and when is it not set?

and how does this interact with "CSS needs to dynamically pick the height of the diary-card style for that as well"

For the CSS, if the problem is recalculating the height of the diary-card, seems like you should be able to make be calculating using a function as well instead of using a %.

@sebastianbarry
Copy link
Author

I also noticed the map was not stretching to fit the div, so this is what I worked on:

Screen Shot 2022-11-07 at 10 32 59 AM

Screen Shot 2022-11-07 at 10 33 37 AM

As you can see, the map now dynamically stretches to fit the trip item div. This required a bit of restructuring of the div structure within the trip_list_item.html. You may also be able to tell, the map displays slightly strangely, where sometimes there is a little whitespace below the map, and sometimes the map displays past the bottom of the div. This is because I set it with a percentage, but that varies for each trip and for each device:

/* ----------- iPhone 6+ ----------- */

/* Portrait and Landscape */
@media only screen 
  and (min-device-width: 414px) 
  and (max-device-width: 736px) 
  and (-webkit-min-device-pixel-ratio: 3) { 
    .angular-leaflet-map { position: absolute; height: 82.8%; width: 46.8%; }
}

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

@sebastianbarry from the ionic1 code, it looks like what we really want is to invoke refreshDimensions when the data changes.

refreshDimensions is invoked in many instances, but the easiest to hook into in our case is likely

    scrollCtrl.$element.on('scroll-resize', refreshDimensions);

so if we can get the element similarly in our directive and call scroll-resize on it, we should be able to invoke refreshDimensions

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

so the on listener is through jquery since it is invoked on $element
https://api.jquery.com/on/

As you can see from the jquery event documentation, the way to pass an event to an element is using trigger
And since the label is changed within a directive, it seems like you should be able to get the scrollCtrl like they do in the collection-repeat directive.

So a high level, it seems like you should be able to do:

  1. add require: '^^$ionicScroll', to the documentation
  2. in the directive, access the scroll element scrollCtrl.$element e.g. https://docs.angularjs.org/guide/directive
  3. every time the mode changes to/from e-bike, trigger a resize scrollCtrl.$element.trigger('scroll-resize')

This is intended to be a rough outline of the direction to follow. Please read the ionic source code and the angular and jquery documentation in detail to understand what you are doing. And if this doesn't work, we may want to change the format of the item so that it does not need the height to be changed - e.g. can we display the buttons side by side instead of one above the other so that the width changes but not the height?

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

Note that with require, you may need to use a link function instead of a controller in the directive. the angular documentation does not seem to show how required controllers are bound to another controller. You might want to experiment with the values passed in to the controller first - e.g. $scope, $element, $attrs, but change if none of them give us a reference to the other controller.

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

Per http://websystique.com/angularjs/angularjs-custom-directives-controllers-require-option-guide/
it looks like you might need the link after all.

But maybe you don't need to find the element to notify it after all. angular's event mechanism is much more sophisticated than jquery and does not need you to have the element. Instead, you can pass events up and down the DOM tree. So maybe something as simple as$scope.$emit('scroll-resize') will work?
https://docs.angularjs.org/api/ng/type/$rootScope.Scope#$emit

Or maybe not; it looks like the two eventing mechanisms are completely different
https://stackoverflow.com/a/16511424/4040267

But collection-repeat is an angular directive, can we send the data directly to it?

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

Final comment.

It does not look like the collection-repeat listens to any angular events other than $ionicExposeAside and $destroy, neither of which is what we want.

However, it does listen to angular.element($window).on('resize', onResize);, which opens the option of angular.element($window).trigger('resize') instead of importing the controller and changing the link function.

Note however, the on window resize, it calls onResize, which only refreshes the dimensions if scrollView.__clientWidth, scrollView.__clientHeight are different, which they will not be.

However, looking at the implementation of refreshDimensions, it looks like in this dynamic case we just call getRepeatManager.refreshLayout

      // Dynamic dimensions aren't updated on resize. Since they're already dynamic anyway,
      // .getValue() will be used.

      getRepeatManager().refreshLayout();

so I thought that maybe we could inject the repeat manager, similar to

.factory('$ionicCollectionManager', RepeatManagerFactory);

get our own copy of it

    function getRepeatManager() {
      return repeatManager || (repeatManager = new $ionicCollectionManager({
        afterItemsNode: afterItemsContainer[0],
        containerNode: containerNode,
        heightData: heightData,
        widthData: widthData,
        forceRefreshImages: !!(isDefined(attr.forceRefreshImages) && attr.forceRefreshImages !== 'false'),
        keyExpression: keyExpr,
        renderBuffer: renderBuffer,
        scope: scope,
        scrollView: scrollCtrl.scrollView,
        transclude: transclude
      }));
    }

and call refreshLayout on it.

Not sure this will work if all the elements are required to pass in, but if some of them are not used in renderLayout, maybe we can make it work as well.

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

@sebastianbarry I have outlined three possible approaches:

  • using scrollControl.$element.trigger (this seems the easiest to use honestly)
  • experimenting with $scope.$emit (unlikely to work)
  • injecting RepeatManagerFactory and creating our own copy to call refreshItem

If none of those are easy to use, our options are:

  • editing the ionicframework code to expose what we want (if the method we want exists but is not exposed)
  • changing the structure of the item so we don't need to change the height (for example, could the mode and replaced mode buttons be side by side instead of one below the other).

Try them out and LMK what works.

@shankari
Copy link
Contributor

Here's a proposed patch with a lot of comments.
Hopefully that is enough to unblock you.
add_adjust_height.patch.gz

@sebastianbarry
Copy link
Author

sebastianbarry commented Nov 14, 2022

I have spent today morning trying to get the patch to work, and while I see how it is supposed to work, I think my knowledge of Angular transclude, require, and link are keeping me from making much progress on this. Without changing the code you sent in the patch and just rearranging/commenting out parts of the code, I was not able to get the <multilabel> directive to render with any parts of these changes. See below for my different thought processes in testing:


First, here is the proper render, with all of the changes in the patch commented out:
Screen Shot 2022-11-14 at 12 10 55 PM


Then, I just tried requiring multilabel. To my knowledge of Angular's require, this means that Angular will give a compiling error if a multilabel element is not present within this directive as a child. It makes sense to me that this will not work (aka the multilabel directive will not render, since there is no reason for multilabel to have a multilabel child within it, but this brings me onto transclude later on).

(from https://docs.angularjs.org/guide/directive) When a directive uses this option (require), $compile will throw an error unless the specified controller is found.

Screen Shot 2022-11-14 at 12 11 20 PM


Then, I just tried requiring ^^$ionicScroll. To my knowledge, this means that Angular will give a compiling error if $ionicScroll is not inside of some parent element's directive's controller parameters. For example, if our <multilabel> element is a child of <parent> directive, and in the .controller('Parent', function( $scope, $ionicScroll, ..., then we will not get a compiling error. What I am unsure of, is whether this require statement, now inputs the $ionicScroll from that parent's controller parameter, as a usable object in our directive that we require'd it in. Essentially, this will allow us to access scrollCtrl from $ionicScroll in our

function postLink(scope, element, attr, scrollCtrl, transclude) {
    element.on('height-change-needed', scrollCtrl.$element.trigger('scroll-resize'));
  };

inside the mutlilabel directive, like exactly how you wrote in your patch @shankari . Why do we not have to specify $ionicScroll.scrollCtrl?
Screen Shot 2022-11-14 at 12 11 55 PM


Then, I tried the transclude: 'element' along with require: '^^$ionicScroll', but I am not sure how Angular direcgive transclude works. To my knowledge from reading this document (https://docs.angularjs.org/api/ng/directive/ngTransclude)
transclude is typically set to true, so that when we have

<multilabel>
  <a> Hello </a>
</multilabel>

It gets rendered to

<multilabel>
  <multilabelcontent... />
  <a> Hello </a>
</multilabel>

Instead of just

<multilabelcontent... />
<!-- Notice how the Hello has disappeared and the heading element for the directive <multilabel> has disappeared too -->

However, I am not sure how transclude works if you say transclude: true instead of transclude: 'element'. I see that we use transclude as a parameter in the postLink, does this mean that by transcluding element, it makes transclude = true, and then also allows us to get the element as an argument in our postLink function?
Screen Shot 2022-11-14 at 12 12 09 PM


Finally, I tried your suggestion of using require: ['^^ionicScroll', 'multilabel'], but the directive still does not render and this I am not sure why, other than the previous attempts didn't work and this is a union of both of them:
Screen Shot 2022-11-14 at 12 12 46 PM
Screen Shot 2022-11-14 at 12 13 07 PM


@shankari do you see a flaw in my thought process? I was trying to learn this by looking it up on my own through the angular documentation, but do you see if I'm looking in the wrong places or looking for the wrong thing?

@shankari
Copy link
Contributor

First, did you look at the console logs to see if there are any errors? If the directive is not rendering, there is an error.
What is it? As I indicated, I wrote the patch to give you a sense of what the code could look like; I make no guarantees that it is perfect.

inside the mutlilabel directive, like exactly how you wrote in your patch @shankari . Why do we not have to specify $ionicScroll.scrollCtrl?

Because the documentation for link says that if you require a directive, the controller for the directive is passed in to the link function.

What I am unsure of, is whether this require statement, now inputs the $ionicScroll from that parent's controller parameter, as a usable object in our directive that we require'd it in. Essentially, this will allow us to access scrollCtrl from $ionicScroll in our

Did you try to add log statements or access through the debugger to see what the scrollCtrl object passed in to the link function is?

@sebastianbarry
Copy link
Author

First, did you look at the console logs to see if there are any errors? If the directive is not rendering, there is an error.
What is it? As I indicated, I wrote the patch to give you a sense of what the code could look like; I make no guarantees that it is perfect.

There does seem to be an error during the compile stage for 'multilabel':
Screen Shot 2022-11-15 at 9 51 51 AM
Screen Shot 2022-11-15 at 9 52 05 AM

Following the link to that issue, we get this Angular documentation page which is not much help:
Screen Shot 2022-11-15 at 9 52 22 AM

I believe this is an issue with my understanding of Angular, and how the different modules interact. I found this page which has been helping me to understand how it works, but I still don't know how to execute it in the code: https://stackoverflow.com/questions/20018507/angular-js-what-is-the-need-of-the-directive-s-link-function-when-we-already-ha

@shankari
Copy link
Contributor

shankari commented Nov 15, 2022

this is an easy fix - the require doesn't go inside the scope, it goes outside the scope
https://github.com/ionic-team/ionic-v1/blob/eefba1331bc9206767c651912db71e9ed34850d5/js/angular/directive/collectionRepeat.js#L98
and is at the same level as the link function

That's what "invalid isolate scope" means - the isolated aka modular scope that you are creating is invalid.

@sebastianbarry
Copy link
Author

Here is the patch I had with my previous changes to make the trip-item height dynamic as opposed to singularly-static to the original 330px:
sebastian_11-15-22.patch.txt
(had to change extension from .patch to .patch.txt for github)

Changes:
main.diary.css

  • Changed the way the map displays, so that it is variable and changes with the height of the trip item
  • Changed the way the diary card displays as min-content, dynamic, instead of a static hard-coded pixel value

infinite_scroll_list.js

  • Added a function that returns a hard-coded pixel height value for the trip item in the template's collection-repeat

infinite_scroll_list.html

  • Added the dynamic collection-repeat "item-height" attribute that is calling the function we made in the controller

trip_list_item.html

  • Added some styling to the elements so that they display properly with the dynamic height now

@shankari
Copy link
Contributor

@sebastianbarry if I apply your patch, then there is a fair amount of gap between adjacent trips, and the gap is filled in if we select e-bike. Is this what you expected? Why is this not good enough?

height_already_changes_dynamically.mov

@shankari
Copy link
Contributor

So here's what I see:

  • when we have only two labels, the height is 391 (as expected), but the item height is actually bigger than the card size
  • when we have three labels, the height is still 391, but the card size expands to fill the full item height

So can you clarify why this is not good enough?

Screen Shot 2022-11-15 at 3 26 10 PM

Screen Shot 2022-11-15 at 3 26 38 PM

@shankari
Copy link
Contributor

shankari commented Nov 16, 2022

That works. Please see attached video and focus on the second collection-repeat item.

item_height_change_dynamically.mov

I have also attached a patch of all the changes (including @sebastianbarry's and my changes).

@sebastianbarry can you take over now and finish testing?
Make sure that this works in all locations where we use the linkedsurvey - at least the diary, label, diary detail and label detail screens.

Also, it would be good if you would sent the scroll-resize even only when the to or from mode was e-bike

Also, can you answer my questions about the item-height versus the card height above? It looks like the replaced mode is not cut off even without changing the item-height because the card height changes dynamically. So why do we need to change the item height?

shankari added a commit to shankari/e-mission-phone that referenced this issue Nov 16, 2022
…is displayed

Changes by @sebastianbarry

- main.diary.css
    - Changed the way the map displays, so that it is variable and changes with the height of the trip item
    - Changed the way the diary card displays as min-content, dynamic, instead of a static hard-coded pixel value

- infinite_scroll_list.js
    - Added a function that returns a hard-coded pixel height value for the trip item in the template's collection-repeat

- infinite_scroll_list.html
    - Added the dynamic collection-repeat "item-height" attribute that is calling the function we made in the controller

- trip_list_item.html
    - Added some styling to the elements so that they display properly with the dynamic height now

Changes by @shankari

- Find the closest ancestor scroll element if present
- Cache it for future use
- Call scroll-resize on it

Testing done:
e-mission/e-mission-docs#831 (comment)

Pending testing:
- test in all locations where we use the linked survey
    - at least infinite scroll, diary, diary detail and infinite scroll detail
    - also include others found through a recursive search

Pending improvement:
- only trigger the resize if the original value or the new value is e-bike

Testing done:
Connected via the inspector and verified that the item height changed when the
mode was changed to/from e-bike. The distance between items also remained the
same, unlike with the prior implementation where the card size changed, but the
item size did not, so the gap between items narrowed.
@shankari
Copy link
Contributor

shankari commented Nov 16, 2022

Here are my changes, as they get more complex, I thought it would be easier to commit them into a branch than send patches around: e-mission/e-mission-phone#911

Please let me know when you are done with the testing/minor additional fix.

@sebastianbarry
Copy link
Author

Here is the PR which contains the testing on 3 different devices, and my minor additional fix for the sizing of the gap between trip items: e-mission/e-mission-phone#912

@shankari
Copy link
Contributor

@sebastianbarry did you note my request for the additional fix - resize only if the to/from mode is e-bike...

@sebastianbarry
Copy link
Author

@sebastianbarry did you note my request for the additional fix - resize only if the to/from mode is e-bike...

I did not take a look at that. Let me give it a try and see if I can make that change now

@shankari
Copy link
Contributor

@sebastianbarry also please see the additional testing requested in my commit. I write long commit messages for a reason 😄

@sebastianbarry
Copy link
Author

sebastianbarry commented Nov 17, 2022

I added the change to resize only if the to/from mode is e-bike: e-mission/e-mission-phone@a7b6bc3

I am curious to know if this works when the app is on a study

To-Do:

  • Test the resizing of selected mode "e-bike" when the app is on a study -> this should do nothing as we don't need to resize when app is a study

@sebastianbarry
Copy link
Author

I tested the resizing change on the following pages:

  • infinite scroll (working)
  • diary (not working)
  • diary detail (not working)
  • infinite scroll detail (not working)
infinite scroll diary diary detail infinite scroll detail
Simulator Screen Shot - iPhone 11 - 2022-11-17 at 11 36 11 Screen Shot 2022-11-17 at 10 08 10 AM Simulator Screen Shot - iPhone 11 - 2022-11-17 at 10 08 18 Screen Shot 2022-11-17 at 10 08 54 AM

The leaflet map appears to not be influencing the other elements on screen according to its own height. AKA, the div that comes right after the leaflet map, is displaying at the very top of the leaflet map and is being covered by the leaflet map.

@sebastianbarry
Copy link
Author

I found why this is the case; because of the changes I made to the css for angular-leaflet-map to make the map display dynamically:

The old way The changes I made way
.angular-leaflet-map { height: 150px; width: 245px; } .angular-leaflet-map { position: absolute; height: 82.8%; width: 46.8%; }
Simulator Screen Shot - iPhone 11 - 2022-11-17 at 12 11 09 Simulator Screen Shot - iPhone 11 - 2022-11-17 at 12 13 18
Simulator Screen Shot - iPhone 11 - 2022-11-17 at 12 11 03 Simulator Screen Shot - iPhone 11 - 2022-11-17 at 12 13 22

@shankari
Copy link
Contributor

yes, you should definitely test that the e-bike mode does not change anything on studies.
Is it possible to have separate css classes for the diary and detail views?
Note also that I used to have the map in the infinite scroll detail as a % and it would not display on motorola and iOS. I had to convert it to a hardcoded value

iOS: e-mission/e-mission-phone@089797e
android: e-mission/e-mission-phone@a933410

@sebastianbarry
Copy link
Author

sebastianbarry commented Nov 21, 2022

The problem with the way the diary card is displaying currently is that:

In the Diary card, we are currently using 2 div columns separated by a float: left/right
Screen Shot 2022-11-21 at 9 49 36 AM

Apparently, when you use float, the element ignores the height/width of it's parent element:

Why is the parent div height zero when it has floated children
Ordinarily, floats aren't counted in the layout of their parents. To prevent that, add overflow: hidden to the parent.

Adding overflow to the parent in this case does not magically cause the floated div to fill to the height of the parent:
Screen Shot 2022-11-21 at 9 50 35 AM

We may have to restructure the way the template is written for this trip list item, so that we don't need to use float.

Two-column layout without floating elements?
You have a few options...
Absolute positioning COL1 width:50% left:0% COL2 width:50% left:50%
Fixed positioning "same as above but will interfere with scrolling"
A table layout, even though purists will protest, it will work correctly

I tried absolute positioning, but this still does not fill the div to be the height of the parent

A lot of these guides I'm finding online work when they're using text, because text has an inherent pixel height, but we NEED this div to be the height of the diary card, because the leaflet map will fill to the height of whatever it's parent div is. If the height of the diary map parent is 0, the map will have height 0 too and not display

I think a table layout would be suitable for this (as seen in the quote above from Stack Overflow), but I've never done it before and I am not sure the pros/cons to doing it that way/. @shankari do you have any ideas on this?

@shankari
Copy link
Contributor

Again, given that we will remove the diary tab later, can we just create multiple css types - one for label and one for diary?
This will allow us to keep the original hardcoded, non-% values for diary and the new % based values for label.
Note that in the diary screen, the item height never changes.

@sebastianbarry
Copy link
Author

I was able to get the map to display properly in both places without using additional css types - this is because they were already using different css types. The label screen was using diary-map while the detail screen was not. All we needed was to make sure the detail screen was not affected by any changes we make to the label screen, by adding styling to the css-specific-to-the-label-screen, and the template-specific-to-the-label-screen.

Testing:

Screen.Recording.2022-11-21.at.11.03.28.AM.mov

Simulator Screen Shot - iPhone 11 - 2022-11-21 at 11 03 59

@sebastianbarry
Copy link
Author

sebastianbarry commented Nov 21, 2022

Further testing:

Program Study
Simulator Screen Shot - iPhone 11 - 2022-11-21 at 11 33 27 Simulator Screen Shot - iPhone SE (2nd generation) - 2022-11-21 at 11 49 33
Simulator Screen Shot - iPhone 11 - 2022-11-21 at 12 01 28 Simulator Screen Shot - iPhone SE (2nd generation) - 2022-11-21 at 12 00 48
Simulator Screen Shot - iPhone 11 - 2022-11-21 at 12 01 19 Simulator Screen Shot - iPhone SE (2nd generation) - 2022-11-21 at 11 49 50
Simulator Screen Shot - iPhone 11 - 2022-11-21 at 12 01 14 Simulator Screen Shot - iPhone SE (2nd generation) - 2022-11-21 at 12 00 31

@shankari
Copy link
Contributor

shankari commented Nov 21, 2022

@sebastianbarry I don't see the e-bike being selected in any of the detail screens. You have to think through and test all combinations 😄

@sebastianbarry
Copy link
Author

@sebastianbarry I don't see the e-bike being selected in any of the detail screens. You have to think through and test all combinations 😄

I updated the testing screenshots with screenshots of "e-bike" as selected.

One thing I noticed: The replaced mode button is displaying and getting cut off by the bottom of the diary screen

We can dedicate time to fix this, but since we are planning on getting rid of the diary screen anyway, we may want to decide to label this as a defect

@shankari
Copy link
Contributor

shankari commented Nov 21, 2022

One thing I noticed: The replaced mode button is displaying and getting cut off by the bottom of the diary screen

@sebastianbarry Ah yes, I know that people in my family still sometimes use the diary screen. I think we should fix it.

Since we made most of the changes to the multi-label-ui directive, they are already in the diary as well. So this should be as simple as making the item height of the diary screen also be dynamic.

@sebastianbarry
Copy link
Author

Regarding the diary screen replaced mode labeling:

iPhone 11 (program) iPhone SE 2nd gen (program) Android Pixel 6.0 (program)
https://user-images.githubusercontent.com/61334340/203371875-408e30e9-cefb-4911-8cd0-0952848a2e9b.mov https://user-images.githubusercontent.com/61334340/203372020-317c518c-0b43-4994-b7ff-8f7c5850f6f0.mov Screen Shot 2022-11-22 at 9 40 32 AM
https://user-images.githubusercontent.com/61334340/203371940-6522a066-a04b-4180-9cde-6db5d24e1e1f.mov

As you can see, the iPhones are working well, even with drastically different pixel dimensions the diary displays well and responds to the program e-bike height on the diary list screen and diary detail screen.

The Pixel on the other hand, is not displaying the diary screen at all. I am not sure if this is because of the change I made or if I'm not setting it up right. It is displaying label screen trips, but the app cannot get all green checks on the app status screen because the location says the permissions aren't set right. Usually this hasn't been a problem, but I think it may be a problem for the Android

Screen Shot 2022-11-22 at 9 35 23 AM Screen Shot 2022-11-22 at 9 35 29 AM Screen Shot 2022-11-22 at 9 35 54 AM
Screen Shot 2022-11-22 at 9 40 24 AM Screen Shot 2022-11-22 at 9 40 32 AM

I looked through the logs and I am not seeing any logging errors related to the diary screen loading, so not sure where this issue is coming from:

Screen.Recording.2022-11-22.at.9.51.57.AM.mov

The only error coming up is this:

image

And it only comes up when I try to click or scroll on the screen

@shankari
Copy link
Contributor

The app status should not affect the diary screen. Having the diary not show up at all is pretty bad.
When is the last time that the Pixel worked for you?
Have you tried running only the Pixel and not the iPhone?
Have you tried restarting the Pixel?

@sebastianbarry
Copy link
Author

I don't believe I've ever tried or seen the diary screen working on the Pixel.
I am restarting it now and only running the Pixel to see if that works

@sebastianbarry
Copy link
Author

sebastianbarry commented Nov 23, 2022

I just tried the Pixel on the master branch and for the first 30 seconds it still didn't load:

But after 30 seconds it magically loaded and let me see the trips:

Working on master Loaded Seeing the trips
image Screen Shot 2022-11-23 at 7 14 48 AM Screen Shot 2022-11-23 at 7 15 06 AM

Testing it out on my branch: It is not working, diary screen is not displaying. The only error I see is this:
image

And possibly any of these:

image

@sebastianbarry
Copy link
Author

I got it to work with a different Android emulator!

Screen.Recording.2022-11-23.at.9.14.59.AM.mov

Android looks good this change should be good to go

@shankari
Copy link
Contributor

Great! Merging and deploying today. You should be able to see the change on the test phones if you happen to have them with you.

@shankari
Copy link
Contributor

shankari commented Dec 7, 2022

Fixed and merged and tested.

@shankari shankari closed this as completed Dec 7, 2022
Repository owner moved this from Current two week sprint to Sprint tasks completed in OpenPATH Tasks Overview Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants