Skip to content

Conversation

@weegeekps
Copy link
Contributor

@weegeekps weegeekps commented Apr 8, 2022

This in-progress PR contains the major UI improvements for V2.

  • Migrate to Tailwind CSS.
  • Move the filter bar in a desktop layout to the left.

@weegeekps weegeekps marked this pull request as draft April 8, 2022 19:06
@weegeekps weegeekps requested a review from javagl April 8, 2022 19:06
@weegeekps
Copy link
Contributor Author

@javagl Here's the PR with the latest for the V2 changes. You'll need to run this locally, and your mileage may vary. Please let me know if you find any issues.

@javagl
Copy link
Contributor

javagl commented Apr 9, 2022

I probably cannot do a sensible "review" here.

The actual changes here mainly refer to the UI. I had to do a websearch what "tailwind" actually is, but bailed out when I saw that this is just another framework that wraps the unbearable complexity of CSS away (not necessarily into something that is less complex, but maybe just complex in a different way...). When it comes to style and appearance, I'm more on the ... let's say 'pragmatic' side of things (c.f. https://javagl.github.io/GLConstantsTranslator/GLConstantsTranslator.html ).

I have seen that there are some structures prepared for the last checkbox, namely for the "Details View", but am not sure about the current state of this.

I started looking at the code, but also might need a refresher for React in general: For some things, I'm not sure whether this is part of a reviewable design- or implementation choice, or whether they are just "that's how we roll" in React.

However, more on the ""backend""/structural side:


<brainstorming>

I initially wanted to post the following block in #122 , because it is rather "brainstorming" - but not in terms of functionality, but rather in terms of "architecture", so it did not really fit there. I hope it's OK to drop this here...

I saw that there are a few things that are probably related to #144, namely, a fetchExtensions function in the DataService.ts, and inputExtensions/outputExtensions in the IProjectInfo.

NOTE: This part was unrelated to the pure UI changes here, and has therefore been moved to #151

</brainstorming>


A few things that could actually be part of a "code review":

  • There are not nearly enough comments in the code
  • FilterActionTypes.PERFORM_SEARCH is not used
  • IUpdateSelectedFiltersAction and related elements are not used

(but maybe these are due to the 'draft' state)

@weegeekps weegeekps marked this pull request as ready for review July 22, 2022 13:28
@weegeekps
Copy link
Contributor Author

@javagl As discussed, I've moved the detail screen and extensions work into a branch and cleaned this one up so we can merge it. Please take a look and let me know if you have any questions.

@javagl
Copy link
Contributor

javagl commented Jul 23, 2022

I had a look at the updated state (i.e. the changes), but as I mentioned above, I'd need to dive deeper into React in order to be able to identify any issues here (beyond the high-level ones, like 'There are no comments'). However, I started the updated state locally, and tried it out, including wild clicking and filtering and changing the window size (c.f. useWindowDimensions), and everything seems to work as it should.

OK for me to merge this. I'll carve out some of the 'brainstorming' above into a dedicated issue.

@weegeekps
Copy link
Contributor Author

Thanks, @javagl. I am going to go ahead and merge this into the repo now.

@weegeekps weegeekps merged commit 17e0cef into KhronosGroup:main Jul 28, 2022
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.

2 participants