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

[Next] Major Refactoring, Bug fixes and UI Overhaul #229

Merged
merged 66 commits into from
Jul 21, 2021
Merged

[Next] Major Refactoring, Bug fixes and UI Overhaul #229

merged 66 commits into from
Jul 21, 2021

Conversation

zrthxn
Copy link
Member

@zrthxn zrthxn commented Jul 2, 2021

Summary of Changes

A list of all changes is given here. Screenshots are provided in comments to give temporal relation to commits.

Plugins List

  • Group plugins by version, dont show a new entry for every version
  • Create plugin view, remove plugin preview
  • Implement pagination in plugin list

Dashboard

  • Fix Table layout and reduce complexity
  • Improve UI of plugin list on dashboard

Create plugin form

  • Improve UI of form
  • Check if name is unique while typing
  • Fix issues with file upload component

Plugin body

  • Improve UI and layout
  • Fix issues caused by removal of PF3

Fixes: #203
Fixes: #101
Closes: #197
Fixes: #198
Closes: #200
Fixes: #227
Fixes: #225
Partially: #214
Fixes: #168

Problems Remaining

  • Versions are not fetched in call for PluginMetas. This versions content is empty.

Preview

Screenshot (83)
Screenshot from 2021-07-03 02-06-56
Screenshot (86)
Screenshot from 2021-06-05 00-58-23
Screenshot from 2021-06-05 01-01-27

Copy link
Collaborator

@jennydaman jennydaman left a comment

Choose a reason for hiding this comment

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

630a0ac...ac86d12 LGTM

Do you want to implement #168 before merging? (eslint --fix will be very disruptive)

@zrthxn
Copy link
Member Author

zrthxn commented Jul 6, 2021

Can be done.
But running yarn lint (eslint ./src) doesn't give any problems? Why is that?

@jennydaman
Copy link
Collaborator

I meant, after AirBnb and/or Prettier are put into effect, the lint fixes will be disruptive. Current code is probably in-compliance with the default create-react-app's eslint config.

BTW in .eslintrc, .js is listed as an allowed react/jsx-filename-extension — do we want to remove this?

@zrthxn
Copy link
Member Author

zrthxn commented Jul 6, 2021

Okay got it.

We can continue allowing .js, it doesn't really cause any problems. Just that the chances of someone writing in a bug are slightly higher because there's less hinting

@jennydaman
Copy link
Collaborator

For the sake of consistency, I think we should require that all JSX have the JSX extension.

(Maybe it's possible to allow for JS without react files, e.g. helper, model, or controller modules, use the .js extension but I'm not sure if eslint is smart enough for this)

@zrthxn
Copy link
Member Author

zrthxn commented Jul 6, 2021

@jennydaman
The Airbnb style guide requires that every prop should be defined in props validation. This creates a huge disruption and will need quite a lot of time to fix since each prop has to be manually fixed.

➜  ChRIS_store_ui git:(next) ✗ yarn lint:fix
yarn run v1.22.10
$ eslint --fix src/**/*.jsx
...
✖ 168 problems (165 errors, 3 warnings)

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I suggest we should disable the react/prop-types rule. What do you think?

@jennydaman
Copy link
Collaborator

Disabling react/props-types "for the time being" sounds like a good plan.

@zrthxn
Copy link
Member Author

zrthxn commented Jul 8, 2021

@jennydaman
Added the eslint config to .eslintrc, added .prettierrc.
Installed @babel/eslint-parser and @babel/preset-react to allow eslint to use babel.
Cleaned up the hundreds of linting errors.

Screenshot from 2021-07-08 15-21-06

@zrthxn
Copy link
Member Author

zrthxn commented Jul 8, 2021

@jennydaman @mairin
What was the review for #210? Can we merge that into this branch to get that redesign done?

@jennydaman
Copy link
Collaborator

Hmm, I remember that we supported the new home. If you'd like, merge the redesigned home into next and I'll take a look at it.

@zrthxn
Copy link
Member Author

zrthxn commented Jul 8, 2021

@jennydaman Merged the redesigned home

@jennydaman
Copy link
Collaborator

jennydaman commented Jul 8, 2021

I'll review the UI in a demo before diving a bit into the code.

image

  • Would you have the "Sign in/out" button blue?
  • IMO the search bar could use some contrast balance, make the placeholder text brighter, the icon dimmer
  • Remove "Another" button

@zrthxn
Copy link
Member Author

zrthxn commented Jul 8, 2021

@jennydaman Done these

@jennydaman
Copy link
Collaborator

@zrthxn I'm getting an infinite loop here. Ping me on slack if you want me to look into the code.
record

@zrthxn
Copy link
Member Author

zrthxn commented Jul 19, 2021

@jennydaman
It was happening if the request to api.github.com failed or was blocked, which is what I think you tried with pi-hole. Because of a circular dependency in a useCallback hook. Fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment