search

openaq / openaq-metadata-editor

MIT License
1 stars 1 forks source link

Form validation #34

Closed sethvincent closed 5 years ago

sethvincent commented 5 years ago

closes #18

kamicut commented 5 years ago

A couple of comments:

  1. Pollutants don't seem to be saved? Is this a regression?
  2. It isn't immediately clear what the error is. I would maybe make sure that the required fields have an asterisk next to them, and maybe act on "required" errors by changing the error message to "Make sure to fill in required fields"
sethvincent commented 5 years ago

I'll check on the pollutants.

I'd rather do a follow-up pr that tackles error messages in a more comprehensive way. But yeah I agree with that.

sethvincent commented 5 years ago

I think the pollutants/parameters problem had two parts:

  1. dependency problem. i've been changing the schema in openaq-data-format over the past couple days and yarn is very specific about which commits it pulls when using a git dependency, so i should have mentioned in the pr that a specific commit is needed.
  2. the validation checker got confused because it couldn't find the required properties at all in a new instrument, so the latest commit fixed that.
sethvincent commented 5 years ago

added asterisks to the labels of required inputs.

kamicut commented 5 years ago

@sethvincent I like the new asterisks, I think there is an issue still when clearing pollutants.

  1. Add a new instrument
  2. Add pollutant for that instrument
  3. Save
  4. Re-edit
  5. Clear the pollutants in both instruments
  6. Save
  7. It will go through, the page says that there are no pollutants, but when refreshing, the pollutants are re-added to the instruments 👻