art-institute-of-chicago / browser-extension

Browser extension to view a random artwork from our collection in a new browser tab
GNU Affero General Public License v3.0
14 stars 6 forks source link

Feature/date filter #49

Closed dylanirlbeck closed 4 years ago

dylanirlbeck commented 4 years ago

Fixes #48

~This PR is a WIP -- all the features work and a basic design is in place, but I wanted to put it up for feedback in case anyone is curious and can provide some early feedback. The code is pretty ugly, so I plan on cleaning it up.~

This PR adds an option for filtering the generated artwork by a specified date range. I also added some custom validation, to prevent users from entering dates less than -8000 (8000 BCE), greater than 2020 (the current year), or entering a From date greater than a To date.

In addition, I noticed this project is not using a standard formatter for JS/HTML/CSS, so I went ahead and used Prettier. Most of the diff will likely be associated with this reformatting, and won't be actual functional changes.

UI

Note, I'm not a designer. Feedback on the UI is much appreciated! At the moment the form has some basic validations (you'll get a red border around the input and be prevented from submitting) for the following conditions:

image
surreal8 commented 4 years ago

This is great @dylanirlbeck! Thank you! I like that the setting is incorporated into the extension's settings, and not directly on the page. Design/UX wise I think it looks nice and clean, only thing is we probably don't need the scroll bars for this one item, and when you hit 'ok' I'd prefer if the settings box closed, but we should run all this by our designer when she has a moment.

I put a date range between 1995-2020 and I got this 1 artwork, https://www.artic.edu/artworks/128480/arch-of-pantani-plate-thirty-seven-from-the-ruins-of-rome?utm_medium=chrome-extension&utm_source=Arch%20of%20Pantani,%20plate%20thirty-seven%20from%20the%20Ruins%20of%20Rome, whose date data doesn't seem to be correct, right @IllyaMoskvin and @nikhiltri?

1980-2020 yields 3 artworks :).

dylanirlbeck commented 4 years ago

@surreal8 Thanks for taking a look! I second the call for the OK button to close the modal -- I'll see if I can get that to work. I'm not sure the best way to indicate to the user that the options have been saved correctly, but I'm sure that's something we can talk through before merging in to master.

only thing is we probably don't need the scroll bars for this one item,

Can you clarify what you mean here? I'm not seeing scroll bars on my options modal, though I'm not sure if this is what you mean.

I put a date range between 1995-2020 and I got this 1 artwork

That happened to me as well. I'll try and debug the code to see if there's something going wrong on the extension end (as opposed to Elasticsearch).

dylanirlbeck commented 4 years ago

@IllyaMoskvin My apologies for the delay, and I hope all is well with you and the ARTIC! I was actually able to visit last weekend -- l especially liked Malangatana 😄

My internship got very busy, and I was moving all over the place. I believe I've addressed all your comments in the original review, so I think this is ready for you to take another look!

One thing to note: in order to minimize the diff I changed the base branch from feature/settings to master. If you could go in and rebase feature/settings against master to get the Prettier changes it'd be much appreciated! I can then modify the base to point back to feature/settings.