aquariumbio / aquarium

The Aquarium Lab Operating System
http://klavinslab.org/aquaverse/
MIT License
58 stars 15 forks source link

Sample types #508

Closed marikoja closed 3 years ago

marikoja commented 3 years ago

Sample types from hamburger menu View, create, edit, delete.

gnomicosuw commented 3 years ago

does not work as expected.

Once an AlertToast is shown and closed, the AlertToast shows up again every time the user types in an area, which is very confusing and very distracting.

Repro:

  1. navigate to a page with a form
    NOTE: (you can use the new sample type form but that page has other errors which makes it confusing. I can demo on a different page in a different branch)

  2. submit the form, but with errors result: you get the

  3. close the toast

  4. make any change to any input field (such as a text field) result: the shows up as soon as you type in the field.

NOTE: the useEffect code below gets triggered and reopens the toast as soon as you type in an input field (you can repro this with a simple debugger statement). I'm not sure why.

  // Set state from props
  useEffect(() => {
    setMessage(props.message);
    setSeverity(props.severity);
    setOpen(props.open);
  }, [props]);
gnomicosuw commented 3 years ago

Ben, some general comments Mariko & I have discussed that we want your input on.

  1. file naming conventions. Should files names reflect component names? For example,

    • the file "ShowSampleType.jsx" defines the component < ShowSampleType > but
    • the file "SampleTypesPage.jsx" defines the component < SampleTypeDefinitions >
    • the file "samples.js" defines the component < samplesAPI >
  2. attribute naming conventions. Should we try to use the same names when passing and setting attributes? For example,

    • the attribute "testName" gets passed to a component where "cy-data = {testName}" Should we use the same attribute name in both places? Maybe cyData (I think "-" will cause problems, and I think eslint will complain if we use "_")
  3. passing form inputs. In my object_types branch I have used a convention where I define "baseline" form inputs using

    // set formData const form = document.querySelector('form'); const data = new FormData(form); const formData = Object.fromEntries(data);

and then add-on "custom" inputs that after that. In one example,

// add new_samples_private to formData
formData.new_samples_private = samplesPrivate;

I think we should review that strategy as a team before we start implementing a bunch of new forms...

  1. the API interceptor still just tosses javascript alerts to the user on API errors. The "actual" behavior for these errors is still TBD (do we redirect the user to the homepage? do we redirect the user to their previous page? do we show a popup and let the user re-login?, etc...). In some cases the answers to those questions have not been fully spec'd, so Mariko & I suggest just leaving the alerts there for now and dealing with all of them later. I think the three of us agree on that, but it might be worth a quick demo at some point.

  2. Mariko & I came up with a strategy to simulate the equivalent of the Rails "flash[:attribute_name]" feature when we want to show a Toast on the "next" page (such as when saving something on one page and showing "saved" on the next page). We working code is in the "object_types" branch. I think we should review that code and integrate it here.

  3. Mariko & I think we should put the "export" command at the end of each file. (For example, in some cases the PropTypes validators appear after the "export" command - I could go either way on that if you have a strong preference.)

bjkeller commented 3 years ago

On style questions, we are using the Airbnb style guide https://github.com/airbnb/javascript/tree/master/react#airbnb-reactjsx-style-guide

On Fri, Feb 19, 2021 at 10:48 AM gnomicosuw notifications@github.com wrote:

Ben, some general comments Mariko & I have discussed that we want your input on.

  1. file naming conventions. Should files names reflect component names? For example,

Pretty sure this is addressed here: https://github.com/airbnb/javascript/tree/master/react#naming (the answer is yes)

  • the file "ShowSampleType.jsx" defines the component < ShowSampleType

    but

  • the file "SampleTypesPage.jsx" defines the component < SampleTypeDefinitions >

  • the file "samples.js" defines the component < samplesAPI >

    1. attribute naming conventions. Should we try to use the same names when passing and setting attributes? For example,
  • the attribute "testName" gets passed to a component where "cy-data = {testName}" Should we use the same attribute name in both places? Maybe cyData (I think "-" will cause problems, and I think eslint will complain if we use "_")

Can you point me to some code that illustrates what you are asking about? (and as far as names, I would point you to the style guide)

1.

passing form inputs. In my object_types branch I have used a convention where I define "baseline" form inputs using

// set formData const form = document.querySelector('form'); const data = new FormData(form); const formData = Object.fromEntries(data);

and then add-on "custom" inputs that after that. In one example,

// add new_samples_private to formData formData.new_samples_private = samplesPrivate;

I think we should review that strategy as a team before we start implementing a bunch of new forms...

I would have to see concrete code to have an opinion.

1.

the API interceptor still just tosses javascript alerts to the user on API errors. The "actual" behavior for these errors is still TBD (do we redirect the user to the homepage? do we redirect the user to their previous page? do we show a popup and let the user re-login?, etc...). In some cases the answers to those questions have not been fully spec'd, so Mariko & I suggest just leaving the alerts there for now and dealing with all of them later. I think the three of us agree on that, but it might be worth a quick demo at some point.

The alerts are fine until we have error handling worked out. They should be marked with TODO comments, and add a //eslint-disable-line pragma to suppress the no-alert warning. (please don't suppress warnings as a regular thing)

1.

Mariko & I came up with a strategy to simulate the equivalent of the Rails "flash[:attribute_name]" feature when we want to show a Toast on the "next" page (such as when saving something on one page and showing "saved" on the next page). We working code is in the "object_types" branch. I think we should review that code and integrate it here.

OK

1.

Mariko & I think we should put the "export" command at the end of each file. (For example, in some cases the PropTypes validators appear after the "export" command - I could go either way on that if you have a strong preference.)

I don't see that the style guide says anything about it, but appears to export at the beginning of the definition.

Is this something eslint checks?

-- Ben Keller Aquarium Project Lead Klavins Lab Department of Electrical and Computer Engineering University of Washington bjkeller@uw.edu 206-685-6726