-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: main
Are you sure you want to change the base?
Add filtering #458
Conversation
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.
There was a problem hiding this 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:
-
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.
-
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 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.
-
The colour of certain text elements are hard to read when in dark mode. The Instrument column and the Advanced Filters button in particular.
-
The Reduction State and Instruments text for the dropdown filters aren't vertically centred.
-
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.
-
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.
Some files are missing comments and a few could do with their imports being separated for clarity. |
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.
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.
I'll message you today about this because I am not sure what you're referring to.
I'll check this out
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'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. |
I am happy with Title Case I will defer to Material Design in all matters. |
Off the top of my head, the sidebar links are in Sentence case atm. |
Agreed. |
Also my preference for "Sentence case" is that SciGateway uses it in areas that can't be changed. |
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: |
@Dagonite I think I've addressed the issues |
There was a problem hiding this 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:
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:
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.)