Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2320 allow input value copied to selected rows #2345

Merged
merged 34 commits into from
Dec 13, 2023

Conversation

PoornimaVKrishnan
Copy link
Contributor

@PoornimaVKrishnan PoornimaVKrishnan commented Aug 1, 2023

This PR will modify the existing "select all" feature to essentially allow "select some" as we described in #2320 issue. Internally we decided to call this feature/UI element "multi-form-input".

The following is the summary of changes,

  • Since we're not doing "select all" anymore, we changed the name of the corresponding component from SelectAllRow to MultiFormInputRow. This change has been reflected in class names and other related properties.

  • The existing form-container.tsx has three components in it. The file was very hard to read since we had to add more features to the MultiFormRowInputRow. So, this PR will break it into three different files. This way, each file has one component in it.

  • Both FormRow and MultiFormInputRow need to keep track of "active forms". Also if we remove a visible form, we have to update this list. We could do this by adding it to the provider state, which would cause unnecessary renders. That's why we're passing some callbacks around for this purpose between the three components. We might want to clean up the code later, so I left a TODO for it.

  • The multi-form-input is now sticky and takes only the available visible width. So it won't scroll with the rest of the forms (Set all form row is set to view width instead of form container width #2335). To do this, we had to add a max-wdith to the multi-form-input-row that is updated when the width of the FormRow changes (added to form-row.tsx).

  • We didn't like how the input in the multi-form-row stretches and takes up the whole row. That's why we've imposed a maximum width of 1200px with CSS. This CSS change alone for the textarea case as the textarea element doesn't take up the full available width. That's why we had to add a ResizeSensor to multi-form-input-row.tsx. This solution can be a bit annoying as we're updating the width when the container width changes. So, if a user changed the width intentionally, they would have to change it again.

  • Markdown bar in Recordedit app wraps incorrectly #2334

@PoornimaVKrishnan PoornimaVKrishnan requested review from RFSH and jrchudy and removed request for RFSH August 1, 2023 19:58
@PoornimaVKrishnan

This comment was marked as resolved.

@PoornimaVKrishnan

This comment was marked as resolved.

RFSH

This comment was marked as resolved.

RFSH

This comment was marked as resolved.

RFSH

This comment was marked as resolved.

@RFSH
Copy link
Member

RFSH commented Sep 13, 2023

@PoornimaVKrishnan Now that all the changes in this PR are done, we should write test cases for this feature.

Available documents

The following are our test-related docs. You don't need to read all of these initially, but I wanted to have them in one place so you can easily find them and return them. In this case, I already created the skeleton, so you don't need to worry about many details, and you can jump into writing the test cases.

  • I recommend following our step-by-step guide on how to run test cases that is here
  • This document is our main doc for e2e test cases. The following are the sections that might be useful for you:
    • File structure: You can see how the whole "e2e" folder looks like at a glance.
    • Debugging: Goes over how you can use the Chrome inspector client for easier debugging.
  • In here, we go over different topics related to how to write test cases.

Files of interest

As I mentioned, I already pushed a skeleton for this. The following are the files that you should know about :

  • multi-form-input.spec.js: The test spec.
  • multi-from-input.conf.js: This file allows us to run the individual spec. You don't need to change this file. To run this spec, you need to pass the location of this file to protractor. So, assuming you're running this command from the chaise root folder:npx protractor test/e2e/specs/default-config/recordedit/multi-form-input.conf.js
  • data_setup/schema/recordedit/multi-form-input.json: In this file you need to add the definition for the table that you're going to use in the spec. Don't worry about this for now. We can finalize this together once we have the list of test cases.

How to proceed

I recommend following these steps:

  1. Follow the instructions here on how to setup the env variables and run chaise test cases.

    • As I mentioned in our call, you need to create a secondary dummy user. When you've done that, share the cookies of your users with me so I can extend their expire date (so you don't have to keep changing them).
    • Instead of make test just run npx protractor test/e2e/specs/default-config/recordedit/multi-form-input.conf.js
    • Skip running ermrestjs test cases.
  2. Go over the multi-form-input.spec.js file and modify this with describe and it blocks that describe all the test cases that you will eventually implement. When you're done with this, push it and let me know so we can finalize it together.

    • Feel free to change any of the existing ones that I've added inside the create mode. I think the edit mode is done and you don't need to modify that. We should mostly test in "create mode" and the "edit mode" is just to make sure the functionality works and we don't need to go over every scenario.
  3. After finalizing the skeleton, you can start implementing test cases. I recommend running test cases frequently so you can find issues faster instead of implementing the whole thing and then running it.

    • Let me know if you need help implementing a test case. I can help you, and we most probably have done similar tests in other places that I can point them to you. We also have defined some helper functions in the utils folder, which might be useful to you. Namely record-helpers.js, recordedit-helpers.js, and recordset-helpers.js.
    • Avoid using selectors directly in your test cases and instead add them to chaise.page.js. You should also take a look at the existing selectors as we might already have a getter for it in this file.

RFSH

This comment was marked as resolved.

RFSH

This comment was marked as resolved.

@RFSH
Copy link
Member

RFSH commented Dec 2, 2023

@jrchudy This PR is ready to be merged. I updated the description to reflect the latest PR state and have more information.

Changing the selectFile function to return a promise solved the browser.sleep issue I mentioned. I also fixed the bug that we saw related to max-width of multi-form-input-row. I just had to move the ResizeSensor to the proper place.

@jrchudy jrchudy merged commit 20349b3 into master Dec 13, 2023
1 check passed
@jrchudy jrchudy deleted the 2320-allow-input-value-copied-to-selected-rows branch December 13, 2023 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow input value to be copied to selected rows instead of all rows
3 participants