hhaccessibility / hhaccessibility.github.io

http://hhaccessibility.github.io/main/
7 stars 41 forks source link

Location Report: make it so you can expand and collapse the location pop up #244

Open joshi1983 opened 7 years ago

joshi1983 commented 7 years ago

Test feedback showed people are annoyed with big portions of the map on the location report being covered by the question category and rating popup. To address this problem, we want a way to toggle visibility of the popup.

Put an "X" in the popup that will close/hide the popup when clicked.

Make it so that when the marker is clicked and there is no popup, show the popup.

joshi1983 commented 7 years ago

I assigned this to @e1cerebro because he worked on a few other issues related to Google Maps such as #221. After #221 is complete, this may be a good fit for him.

Blandine-AA commented 7 years ago

There is also a bug where the popup isn't vertically positioned to line up with the marker when the browser height is really tall or really short. We'll want to fix that bug at some point too but fixing it as part of this issue is optional.

I think it should be fixed at the same time. It seems more efficient that way. research and be creative on how to have marker right all the time.

Blandine-AA commented 7 years ago

@e1cerebro , before working on this issue, you should first complete the discovery and research on existing apps tasks.

joshi1983 commented 7 years ago

I removed the following from the issue description because this bug has been fixed by the commits just before this comment: "There is also a bug where the popup isn't vertically positioned to line up with the marker when the browser height is really tall or really short. We'll want to fix that bug at some point too but fixing it as part of this issue is optional."

joshi1983 commented 7 years ago

Work on this issue may affect work on #325 in that the toggle would work only when there is sufficient screen width for the pop up to look good on the map.

Blandine-AA commented 7 years ago

Un-assigning Chris since we haven't heard from him for a long time. @joshi1983 , this will need to be the next priority. Can this be done for the 0.0.4 release?

Blandine-AA commented 6 years ago

Requirements for this issue has changed a little bit. See attached document. Location report web page changes.pdf

Blandine-AA commented 6 years ago

Assigned to @princeseth to do the mockup based on the design description in the document attached in previous comment

joshi1983 commented 6 years ago

It would be nice to have a mockup showing each of the following:

Blandine-AA commented 6 years ago

@princeseth, @joshi1983 comments above are just to help you with the deliverables from the requirement/description document that I attached in this thread.

Regarding the mobile/small screen issue, this was already reported in #325.

princeseth commented 6 years ago

@Blandine-AA @joshi1983 I have created three different mockups for marker before it gets expanded, marker is clicked and not logged in and "Personal". Please see attached document Locatio_Mockup_0.4.pdf

Blandine-AA commented 6 years ago

@princeseth, thanks for the effort on making the mockups.

Mockup number 2 is still not correct. The overall information section is not reflecting the design. This is a side by side comparison image image

The "(under development)" flag is not part of the mockup.

For the per category buttons box

Hope these comments help!

princeseth commented 6 years ago

@joshi1983 @Blandine-AA Please, have a look at below mockup 2 and share your feedback. Thanks.! mockup_2updated

Blandine-AA commented 6 years ago

@princeseth, It looks much better. Just note that the pie chart has to be the same type for the overall section and the categories' buttons.

joshi1983 commented 6 years ago

The mock ups look pretty good to me. I like that the "x" is consistently in the top right corner. I guess "Close" will be an alternative option in the lower centre that also can be clicked to close the popups. I'll keep quiet as the discussion continues mainly between Blandine and Prince.

Blandine-AA commented 6 years ago

@joshi1983 , @princeseth, implementation can start on this one as I the other comments are more clarifications. Please ensure that functionalities specifications are implemented as per the design document I provided.

joshi1983 commented 6 years ago

That sounds good.

Prince finished a little git-related task(making a pull request) quite well except that he used the github interface for it.

@princeseth , make a pull request as soon as possible with any trivial code change that you make locally using git commands. Ideally, your implementation is documented with clear git commits so it is better to get the practice with git done before implementation.

joshi1983 commented 6 years ago

The ordering of question categories is complete for this and reviewable at a place like http://app.accesslocator.com/location-report/387. That affected the location report, rating, and user profile.

joshi1983 commented 6 years ago

@princeseth please give a list of the remaining work on this issue and indicate which items you have working from the items not working. I know you mentioned struggling with some CSS changes but do the JavaScript changes work and do they take steps toward looking more like the mockups?

We can use the list to scope out a new pull request on this issue that you can do fairly independently.

princeseth commented 6 years ago

@joshi1983 I have achieved the functionality as per mockups, but struggling with some of the CSS changes.
As of now, the remaining task is to fix CSS for blue box only.

joshi1983 commented 6 years ago

@princeseth I asked for a list but that comment doesn't give one. I want it to be very specific because I'll have to look for specific things when reviewing and know what items are remaining out of your pull request that may be completed in another pull request potentially by someone else. I doubt you'll add the "x" button with CSS so there is at least one non-CSS item remaining. You got the x button to work, right?

princeseth commented 6 years ago

@joshi1983 Below image is similar to the mockups image

On clicking "x" button a map will expand and blue box will get close image

On clicking the marker the blue box will appear again and a map will contract to an earlier position. image

princeseth commented 6 years ago

@joshi1983 I`m struggling with the CSS of the blue box content marked in below image when the name of the place exceeds from an allocated space. image

joshi1983 commented 6 years ago

I wanted to get a comprehensive list of items like mentioned below. This is related to ability to break down problems into smaller sub-problems mentioned in a recent VIP pre-evaluation meeting.

Make a pull request with nothing but the following completed:

One change from what you have in the screenshots is that I want the popup overlapping the map. There won't be a wasteful gap below the popup box if the map is behind it.

The following can be handled in a later pull request after the above changes get merged:

Are there other items that should be added to the list?

princeseth commented 6 years ago

I agree with you regarding popup overlapping the map, but the initial requirement was to open this info window beside the map, not on the top of the map. Blandine mentioned this in the document she provided to me.

In addition to the above list

joshi1983 commented 6 years ago

Can you point out the requirement in Blandine's document that says not to overlap the map apposed to just overlapping part of its right side?

The requirement seems unclear from the PDF. The blue from the map is showing through which is a weak suggestion that it is fine to overlap the map: image

princeseth commented 6 years ago

The third point in the document, she mentioned that info box on the side of the map "not" top. image

You are correct, it is unclear from picture about overlapping the map which creates confusion. But, in the description, it is written not to overlap the map. She also mentioned that map area will reduce and expand accordingly.

joshi1983 commented 6 years ago

@Blandine-AA I suggest we overlap the right side of the map so the ugly gap below the popup box can be filled with the map's zoom + and - features and part of the map. Is that ok?

Blandine-AA commented 6 years ago

I was thinking that the zoom feature would be on the side of the map as it is now. Also, there shouldn't be any gap as the Accessibility info Window will have the same height as the map. Have a look at this Google map where the location info box is on the left. They have the arrow to close and open the section.

Blandine-AA commented 6 years ago

I had a look at @princeseth screen captures above: 1- There shouldn't be a white space below the Criteria categories. See my comment above. 2- the Close button is missing

joshi1983 commented 6 years ago

Matching the height of the map in all cases without leaving an empty gap anywhere might be tricky. Google Maps uses all the space on the side: image

Our popup box content is a lot smaller than that. Maybe we could move the address, phone number, web address information below the categories to fill more space, though.

Which of the following looks best?

Overlapping right side of map: image

On right side of map: image

On right side with gap filled with popup box background colour: image

The close button is the little "X" in the upper right corner of Prince's screenshots.

Blandine-AA commented 6 years ago

Hi @joshi1983 , @princeseth Thanks for all the screenshots and feedback. 1- I need a "Close" button at the bottom of the window in addition to the 'X'. The 'X' is not enough, plus it is barely visible. 2- I think we need to re-arrange the layout of the overall box info as it is too crowded. Looking at the google map example, we could do something similar with the location photo. 3- Looking at the box again. It is too dark. That's why it doesn't look nice in any screenshots. I suggest to redesign with with a lighter shade of blue and the grey. For the size and positioning I would go with something similar to the google map example, but with the zoom feature at the bottom because we have ou info window on the right and not on the left. Is this possible? 4- It is a good idea to move the location details (address, url ...) down in the box. 5- Eventually, the buttons can be a bit larger and wider. I see on @princeseth screenshot that the % is too close to the #rating and #comments info. Also "Public Areas" got too close to the % info. either "Public Areas" is wrapped-up or the window is widened to adjust.

joshi1983 commented 6 years ago

@Blandine-AA yes, I agree on every point there.

We could go lighter by changing the background to a near white with dark text. This would likely be good to get mocked up or screen shot a few colour schemes.

Maybe we should stop having the "X" button at all and just have a clearer "Close" button centered under the categories. That would save some space near the top for long location names. Two ways to do the same thing can get more complex than it needs to be.

joshi1983 commented 6 years ago

@Blandine-AA mentioned that this needs to be done before next demo.accesslocator.com release so this is very important. I'll review the discussion and remaining work needed for this fairly soon.

joshi1983 commented 6 years ago

On mobile devices, the map and buttons will be full screen with no scrolling. On desktop layout, the mix of map and buttons would toggle to show just the map.

Blandine-AA commented 6 years ago

Toggle doen't work properly on desktop. Clicking on the pin should open AND close the info box. The current implementation only allows to open the info box. You have to close using the close button.

Blandine-AA commented 6 years ago

@joshi1983, this is the issue that covers the changes required in the accessibility info box. You even commented on this and summarized the work to be done. Attached is the initial document I created. It has not been updated with the feedback from the discussions in this thread, but the mock-up for the accessibility categories buttons and the overall accessibility section remain. Location report web page changes.pdf

Blandine-AA commented 6 years ago

@joshi1983, the All Comment link in the overall section needs to be implemented in 0.0.5 as part of that feature if it is not too complicated for the time we have. A user may just be interested in the comments provided for that specific location. You want to show them in a separate popup window.

joshi1983 commented 6 years ago

I thought so too. I made a new issue(#515) so @remnantmisook could implement the commenting feature while I'd just do the part of adding the link to the popup when she's done.

Blandine-AA commented 6 years ago

@joshi1983, added P1 label for this to be fixed within the next week.

joshi1983 commented 6 years ago

This: image

was converted to this: image

In the latest pull request.

Blandine-AA commented 6 years ago

@joshi1983, it looks much better. Some feedback:

Question: What happens if the location name is very long? Will the window size adjust accordingly?

joshi1983 commented 6 years ago

If the location name is longer than 30 characters, the text shrinks. It doesn't make the popup larger because a larger popup would look more inconsistent than just smaller text.

The best way you can see the dynamics of it is by finding some long named locations and looking at them on app.accesslocator.com.

Blandine-AA commented 6 years ago

Moving this to 0.0.6 as there is still some work to do on it

joshi1983 commented 6 years ago

We want the phone number on these reports, right? Roughly 80% of locations have phone numbers. We also collect phone numbers in the Create New Location form.

I think the phone number would fit best in the red marked area here: image