Skip to content

!!!!! DO NOT MERGE !!!!#44

Open
dkulp23 wants to merge 155 commits intostartfrom
master
Open

!!!!! DO NOT MERGE !!!!#44
dkulp23 wants to merge 155 commits intostartfrom
master

Conversation

@dkulp23
Copy link
Owner

@dkulp23 dkulp23 commented Nov 9, 2016

No description provided.

Hasnake and others added 30 commits October 25, 2016 18:54
connected stylesheet to html ugh
});
};

// start filtering meal type locations IF CLICKED. saving for now just in case

Choose a reason for hiding this comment

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

it's still best to omit commented code from the master branch. You can always keep it on a feature branch and copy it from there to master if you want to un-comment it.

// ^ end filtering click event for meal types ^

//helper function to create table elements
function makeAnElementWithText(element, textContent, parent) {

Choose a reason for hiding this comment

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

makeAnElementWithText, setClassOfAddressCells, and makeAReservationButton could all be handled by a single function. you could pass the attribute and attribute value as arguments, and just use defaults or an if/else to set no attribute for elements that don't need it.

makeAnElementWithText('th', 'Meal', rowEl);
tableEl.appendChild(rowEl);
for (var i = 0; i < allLocations.length; i++) {
createRow('foodTable', 'tr', 'td', allLocations[i].name, allLocations[i].hood, allLocations[i].address, allLocations[i].restrictions, allLocations[i].mealType);

Choose a reason for hiding this comment

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

so many arguments! why not createRow('foodTable', 'tr', 'td', allLocations[i] here and above,
function createRow(idName, rowElement, El, location) { ... makeAnElementWithText(El, tC1, location.name); ...

makeReservationEventListeners();
showAddressMarker();
var tableEl = document.getElementById('foodTable');
var tableCells = tableEl.getElementsByTagName('td');

Choose a reason for hiding this comment

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

might be easier to hide/show whole s rather than individual cells for filtering

@@ -0,0 +1,195 @@
'use strict';

Choose a reason for hiding this comment

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

looks like this is a duplicate file.

@@ -0,0 +1,36 @@
var breakfastLocationData = [
{'name': 'Millionaire Club Charity Breakfast', 'hood': 'Belltown', 'address': '2515 Western Ave', 'meal': 'breakfast', 'days': [1, 2, 3, 4, 5], 'openTime': 6, 'closeTime': 7, 'restrictions': 'none'},

Choose a reason for hiding this comment

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

it should be possible to flatten this data into one array of locations. there's a lot of duplicate information like name, address, hood for locations that have multiple meals. what if each location object had breakfast, lunch and dinner properties that were either null or breakfast: { days: [1,2,3], openTime: 6, closeTime: 7, restrictions: 'none' }.

var address = event.target.address.value;
var days = [ ];
var displayDays = [ ];
var mondayStatus = function() {

Choose a reason for hiding this comment

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

might be cleaner to put all of these [day]Status functions in one big function since they always get called together. That could also allow you to use a loop to check each day, since the only things that change from one day to another are the selector, the value to push, and the display name. Could be an object like {selector, value, displayName}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants