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

Add filtering #458

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Add filtering #458

wants to merge 30 commits into from

Conversation

keiranjprice101
Copy link
Collaborator

@keiranjprice101 keiranjprice101 commented Feb 21, 2025

closes #392
closes #50

Removed jobs base/ jobs all / jobs general. Replaced with JobTable component which is applied to the single job page, where the jobs updates based on the instrument choice (instrument or ALL)
This has the added benefit for future work with IMAT etc. where a job table is not required, as we will be able to keep the same number of pages, but conditionally render the table vs the imat images etc. based on the instrument selected.

work torwards a more coherent file structure: pages, components, lib (Additional work could be beneficial here still e.g. splitting row from expanded row.)

The HomePage component was moved to a `pages` directory, requiring updates to imports across relevant files. Adjusted asset and dependency paths to reflect the new structure. These changes improve project organization and maintain consistency.
    The InstrumentData file has been relocated to the lib folder for better organization. This change improves the project structure and aligns with modular conventions. No functional changes were made.
Moved Instruments component and updated imports to align with the new directory structure. This enhances code organization and simplifies the pathing for related files.
Introduced a `FilterContainer` component to manage job query filters, including title, experiment numbers, filenames, dates, and reduction states. Supports multi-select, debounced inputs, and clear/reset functionality, enhancing the flexibility and usability of job filtering.
Reorganized component imports and replaced specific job routes with a unified `JobsPage`. Added `LocalizationProvider` with Dayjs adapter for date localization.
Extended the logic to display the config button when the selected instrument is SANS2D, in addition to LOQ and MARI.
Updated the import paths for the ValueEditor component to ensure proper organization within the 'pages' directory.
Copy link
Collaborator

@Dagonite Dagonite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code refactor looks good. The filters look visually okay to me for the most part although I couldn't actually get them working (I assume I need to get a local API branch working).

And other than that I just have a few styling and formatting suggestions:

  1. The Advanced Filters squashes the table down which results in a very limited viewport. Would it makes more sense for the filters to overlap instead maybe? Also the viewport height for the table looks like it's been reduced from what it is on main.

  2. Although I can see it is intentional, most of the columns are centred but the first column and the Title column aren't which I don't think looks right. Does it look better with everything left aligned?

  3. The container for the column headers is really thin and then thickens in scenarios where the words are forced to wrap. Think would look more natural if the height was a fixed value.

  4. The colour of certain text elements are hard to read when in dark mode. The Instrument column and the Advanced Filters button in particular.

  5. The Reduction State and Instruments text for the dropdown filters aren't vertically centred.

  6. The Reduction Inputs look at bit messed up and don't properly fill the empty space for most of the jobs. Also, the right alignment for the values make them hard to read.

  7. A bit nitpicky but I've noticed certain headings have switched to using "Title Case" instead of "Sentence case". Personally think the latter looks better and is what I've been using in other parts of the frontend.

@Dagonite
Copy link
Collaborator

Some files are missing comments and a few could do with their imports being separated for clarity.

@keiranjprice101
Copy link
Collaborator Author

@Dagonite

The Advanced Filters squashes the table down which results in a very limited viewport. Would it makes more sense for the filters to overlap instead maybe? Also the viewport height for the table looks like it's been reduced from what it is on main.

I'll check and compare the height difference. In my opinion, overlapping the table is worse because the filters remain active when the window is collapsed. This forces you to always collapse the filters to view the table, even if you filter down to a single row.

Although I can see it is intentional, most of the columns are centred but the first column and the Title column aren't which I don't think looks right. Does it look better with everything left aligned?

The Reduction Inputs look at bit messed up and don't properly fill the empty space for most of the jobs. Also, the right alignment for the values make them hard to read.

Good catch on the alignments. I meant to review them but got sidetracked by other PR tasks. For now, I'll left-align everything and open an issue to revisit this. Typically, numerical data should be right-aligned and text left-aligned. The reduction inputs and the run details also seem off (the current alignments are difficult to read and hard to scan visually). I’ll revert them to their previous state and include that in the issue.

The container for the column headers is really thin and then thickens in scenarios where the words are forced to wrap. Think would look more natural if the height was a fixed value.

I'll message you today about this because I am not sure what you're referring to.

The colour of certain text elements are hard to read when in dark mode. The Instrument column and the Advanced Filters button in particular.

I'll check this out

The Reduction State and Instruments text for the dropdown filters aren't vertically centred.
Ah yeah, this was a mui issue I never found a solution for. I'll take another look

A bit nitpicky but I've noticed certain headings have switched to using "Title Case" instead of "Sentence case". Personally think the latter looks better and is what I've been using in other parts of the frontend.

I strongly disagree with this. Titles should be in title case. If anything I should change the rest to title case. If you want to tell me what other places this isn't the case I'd be happy to change them as well. @Pasarus thoughts on this

Some files are missing comments and a few could do with their imports being separated for clarity.

I'm surprised we don't have a Prettier or ESLint plugin for auto-sorting and grouping imports. I won't manually adjust them but will open an issue to select and add a plugin.

Regarding missing comments, which ones are you referring to? I also noticed we don’t have a consistent approach to in-code documentation (e.g., JSDoc or TSDoc). It might be worth opening an issue to establish a standard for this as well.

@Pasarus
Copy link
Member

Pasarus commented Feb 27, 2025

The Reduction State and Instruments text for the dropdown filters aren't vertically centred.
Ah yeah, this was a mui issue I never found a solution for. I'll take another look

A bit nitpicky but I've noticed certain headings have switched to using "Title Case" instead of "Sentence case". Personally think the latter looks better and is what I've been using in other parts of the frontend.

I strongly disagree with this. Titles should be in title case. If anything I should change the rest to title case. If you want to tell me what other places this isn't the case I'd be happy to change them as well. @Pasarus thoughts on this

I am happy with Title Case I will defer to Material Design in all matters.

@Dagonite
Copy link
Collaborator

I strongly disagree with this. Titles should be in title case. If anything I should change the rest to title case. If you want to tell me what other places this isn't the case I'd be happy to change them as well. @Pasarus thoughts on this

Off the top of my head, the sidebar links are in Sentence case atm.

@Dagonite
Copy link
Collaborator

I'm surprised we don't have a Prettier or ESLint plugin for auto-sorting and grouping imports. I won't manually adjust them but will open an issue to select and add a plugin.

Regarding missing comments, which ones are you referring to? I also noticed we don’t have a consistent approach to in-code documentation (e.g., JSDoc or TSDoc). It might be worth opening an issue to establish a standard for this as well.

Agreed.

@Dagonite
Copy link
Collaborator

Also my preference for "Sentence case" is that SciGateway uses it in areas that can't be changed.

@Dagonite
Copy link
Collaborator

Dagonite commented Feb 28, 2025

The container for the column headers is really thin and then thickens in scenarios where the words are forced to wrap. Think would look more natural if the height was a fixed value.

I'll message you today about this because I am not sure what you're referring to.

For example, if you click the "Experiment Number" header to sort by it, there's not enough space for the text and the arrow so it word wraps and makes the column headers container thicker. Which is a bit annoying when interacting with the table:

image
image

@keiranjprice101 keiranjprice101 marked this pull request as ready for review February 28, 2025 11:44
@keiranjprice101
Copy link
Collaborator Author

@Dagonite I think I've addressed the issues

Copy link
Member

@Pasarus Pasarus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks and feels real smooth, great work!

Nothing I really want to comment on for the code.

I have noticed that the filters when you click clear, clear from the search but not the actual inputs so you can end up with a state like this:
image

I got an error when I hit "Delete" on the date pickers, I think it has to do with it auto updating when the date pickers don't have 1 of the Day, Month, or Year, replicated by just adding "Month" manually outside the picker:
image

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.

Add filtering to the job base table Reorder file locations
3 participants