fac-14 / Week3TakingAIME

TFL + Giphy API Project
MIT License
2 stars 3 forks source link

Style details #18

Closed MissArray closed 6 years ago

MissArray commented 6 years ago

Restored background colours for each line in CSS. Works on Firefox, Chrome, Edge, Opera.

sima-qian commented 6 years ago

Firstly, I think it's best that we don't do any significant work on the project outside of FAC. It's inconsiderate to everyone else who then has to figure out what a lone individual has done -- particularly when we're going straight into code review tomorrow morning.

It's great that you've got this feature to work (although it doesn't work for me in either Chrome or Safari), and it would be nice to include it in the project. However, there will now be problems with font colour, e.g. the Northern line option has black text on a black background, that you haven't addressed. At this point, when the master branch represents a functional app, I don't think anyone should be merging a faulty version to master.

Secondly, in the future please resolve merge conflicts locally before making pull requests. I've resolved them here, but I won't approve this merge request because of the reasons I outlined above. Adding background colour to the dropdown list items is something we can discuss and work on as a group/pairs after code review tomorrow.

@EmilyScripts / @sbinlondon -- if you disagree, feel free to reopen this pull request and let me know your thoughts.