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

Display invalid markers in a separate sidebar #127

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

mbilker
Copy link
Collaborator

@mbilker mbilker commented Mar 15, 2017

Currently, invalid entries from the data sources are stored without marker.pin and never displayed through a check that marker.isMapped === true.

This PR proposes to display these markers in a separate sidebar on the right side. The design of the entry display in this new sidebar is not final, but the code to display the entries in the sidebar is working.

Copy link
Contributor

@vonbearshark vonbearshark left a comment

Choose a reason for hiding this comment

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

Code looks good, but there doesn't seem to be any unmapped right now? What's that?

index.html Outdated
</div>
<footer>
&copy; 2017 University of Pittsburgh Computer Science Club
</footer>
</div>
<div id="unmappedRecords">
<h3>Invalid Entries</h3>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should call this "Unmapped Records" here, too. Seems more intuitive UX

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do agree that is better.

@mbilker
Copy link
Collaborator Author

mbilker commented Mar 17, 2017

I only saw some unmapped police records, but maybe they are older than 30 days now.

@mbilker
Copy link
Collaborator Author

mbilker commented Mar 17, 2017

Should I add a media query to hide the unmapped records section on mobile?

@mbilker
Copy link
Collaborator Author

mbilker commented Mar 17, 2017

Also, should I make the unmapped records section hideable?

@vonbearshark
Copy link
Contributor

Ideally it should behave just like the other sidebar: shown as closed on mobile with a button to expand/contract on both mobile and desktop

This allows the filtering methods to not care about recomputing the contents of a list cell for an unmapped record.
@mbilker
Copy link
Collaborator Author

mbilker commented Apr 11, 2017

@vonbearshark I see that the table function now returns HTML in the form of <td> elements. Would it be better to return an array?

@mbilker
Copy link
Collaborator Author

mbilker commented Apr 11, 2017

cc @seifriedc ^^

@vonbearshark
Copy link
Contributor

An array? An array of what exactly? The table function is run on each individual cell, not all of them at once, no?

Also, is this ready?

@mbilker
Copy link
Collaborator Author

mbilker commented Apr 11, 2017

@vonbearshark The table method returns td elements like:

<td>text</td><td>asdjfa</td><td>jaldsjfla</td>

I was thinking it could return:

['text','asdjfa','jkasjfdl']

@kingsman142
Copy link
Contributor

kingsman142 commented Jun 1, 2017

@mbilker Are you initial commits associated with "displaying invalid markers in a separate sidebar" ready? If so I can merge this PR and we can worry about returning an array of elements for the table in an issue (I think that's where that problem should go).

In that issue, please detail what we would gain from returning an array of elements like you propose.

…arkers

* upstream/master:
  small word changes to about page
  made the 3rd paragraph of the about this project more vague
  Created rough draft of about page; feel free to add to this
  Jumpstarted the about page for issue pittcsc#65
  Added extra statement to enforce the 'month' button changes to white
  Added functionality to allow users to change the default view of the application to month, week, or day; shipped with week as default
  Added to documentation about changing default checkboxes for including in an external site
  Added functionality so users can choose the default ticked checkboxes
  Added notification animation
  Removed vanish css (for PR)
  Adjusted button positioning
  Remove feedback button
  Add feedback link
  Add feedback button
  Big overhaul. MVP for TPN integration
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