search

FNNDSC / ChRIS_store_ui

UI for the ChRIS Store
MIT License
49 stars 49 forks source link

TODO: Upgrade ChRIS Store from PF3 to PF4 #101

Closed mairin closed 3 years ago

AdamJ commented 3 years ago

Looking at the last time the package.json file was updated, PatternFly Core and PatternFly React are around 2 years old.

The last version of PatternFly React 2 patternfly-react v2.8.0 was released around May 2018. The last version of PatternFly 3 was last released in November of 2018).

@mairin looking at the documentation from PatternFly, as well as the fact that everything is quite old (and hard to upgrade from), what would you like to do to move forward?

mairin commented 3 years ago

@mindreeper2420 what do you think? is it better to rewrite to 4?

AdamJ commented 3 years ago

@mairin IMO, I'd rewrite in 4 (it would open up the UI to newer features/designs, cleaner code, flexibility, etc). PF3 is not supported anymore, so there is also that.

You can run PatternFly 3 and PatternFly 4 together, but that does increase the site size. I've tested out the PF3/PF4 combo and it does run without any noticeable issues, when no changes are applied to the UI. The issues arise when you start to add high-level layout changes (such as to the content wrapper or grid layout), but updating component-level pieces (such as buttons and links) are looking okay.

I've attached a few screenshots - one with high-level changes and one with individual component-level changes.

Page-level changes (content wrapper+cascading styles, buttons, links, lists) Screen Shot 2020-11-09 at 10 55 10 AM

Component-level changes (headers, buttons, links, list) Screen Shot 2020-11-09 at 10 50 09 AM

AdamJ commented 3 years ago

@mairin here is a branch that I tested this on, if you'd like to take a look: https://github.com/mindreeper2420/ChRIS_store_ui/tree/upgrade-chris-store-ui

jennydaman commented 3 years ago

I have been trying to work on this issue but it's dependency hell. @patternfly/react-core@4.75.2 does not work with react@16.4.1, bumping to react@^16.8.0 breaks tests.

However it's not entirely fair that the tests are failing, they are producing errors on Travis as well (yet without failing for some reason)

https://travis-ci.com/github/FNNDSC/ChRIS_store_ui/builds/201177671

console.error node_modules/prop-types/checkPropTypes.js:19
  Warning: Failed prop type: The prop `onStarClicked` is marked as required in `Plugin`, but its value is `undefined`.
      in Plugin (at PluginItem.test.js:8)
jennydaman commented 3 years ago

I disabled the pre-commit hook enforcing tests to be able to push a branch. https://github.com/FNNDSC/ChRIS_store_ui/tree/pf4 The nav bar is replaced by the one provided in Patternfly's react library.

For now there are a few changes and regressions, some due to the default design of Patternfly:

These can be hacked around if need be. Moreover there are some regressions due to time limitations:

image

image

image

mairin commented 3 years ago

Hey @jennydaman, what is the current state of things now - would tackling the regressions you listed in https://github.com/FNNDSC/ChRIS_store_ui/issues/101#issuecomment-726694063 be something that an intern applicant could reasonably work on

jennydaman commented 3 years ago

Summary

I've started an effort doing a part-by-part conversion of Patternfly3 elements to Patternfly4. We should finish this issue before working on other large features. Estimated to be worth 1 month of work.

https://github.com/FNNDSC/ChRIS_store_ui/tree/pf4

Anyone interested in picking this up should contact me.

DivyanshiSingh commented 3 years ago

I would like to help you with this @jennydaman

jennydaman commented 3 years ago

@DivyanshiSingh I think this feature will be a large effort. Do you want to try starting it now, or would you rather save it for later during the main phase of Outreachy?

Though of course, large efforts are also gradual ones. I suppose partial contributions would nonetheless be constructive.

DivyanshiSingh commented 3 years ago

@jennydaman i agree with you, but I would like to start with some contributions as in later phases I will be able to work more efficiently on the same. My recent PRs do require this version change so I am excited to take this up.

DivyanshiSingh commented 3 years ago

@jennydaman what components should I target first? Do you want me to pick random and start or you have some other plan in mind.

jennydaman commented 3 years ago

@DivyanshiSingh I left off on the search widget.

https://github.com/FNNDSC/ChRIS_store_ui/blob/pf4/src/components/Navbar/components/Search/Search.js

I shamelessly stole the code from https://patternfly.org itself. It doesn't work on small screens. But there might be an improvement upstream.

I would suggest you start here

DivyanshiSingh commented 3 years ago

@jennydaman I am planning to take Form next. Also I notice that we don't have forgot password feature, are you planning to bring it in, I can take that in parallel. Let me know your thoughts

jennydaman commented 3 years ago

There is no way of resetting a password besides asking an admin to do it for you. It's a bit complicated for the backend to support an email-password-reset feature because we'd either have to set up a mail server, or use SaaS.

Working on Form sounds like a good way to go.