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

Refactor: Updated location of components. #26

Merged
merged 3 commits into from
Mar 30, 2020

Conversation

dlabaj
Copy link
Collaborator

@dlabaj dlabaj commented Mar 27, 2020

Refactor code to move components into components folder. Updated some of the components name to correspond with where they are used. Added barrel files for imports.

Fixes issue #20

Copy link
Contributor

@mfrances17 mfrances17 left a comment

Choose a reason for hiding this comment

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

Getting some startup errors that are preventing the UI from coming up, are you able to run locally? Looks like a pathing issue. Console screenshot attached:

refactor-issues

function findId(array: Array<any>, id: string) {
var apiTemp = array.find(api => api.id === id);
if (apiTemp != undefined) {
function findId(array: any[], id: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Why use an inline function like this rather than e.g. a static method? Also either way it should probably have a declared return value in the signature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@@ -35,6 +35,7 @@
"jest": "^24.1.0",
"keycloak-js": "9.0.0",
"mini-css-extract-plugin": "^0.4.5",
"nodemon": "2.0.2",
Copy link
Member

Choose a reason for hiding this comment

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

OK so I couldn't figure out why this was added.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lol I love the image. It was originally added because I was working on a fix for for rebuilding the model and services and thought about using nodemon. I'll remove it lol.

EricWittmann
EricWittmann previously approved these changes Mar 27, 2020
Copy link
Member

@EricWittmann EricWittmann left a comment

Choose a reason for hiding this comment

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

Looks good to me other than the enduring mystery of the "No Demon" dependency. :)

Copy link
Contributor

@christiemolloy christiemolloy left a comment

Choose a reason for hiding this comment

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

seeing the same thing as @jenny-s51 and @mfrances17 reported here

@dlabaj
Copy link
Collaborator Author

dlabaj commented Mar 29, 2020

@christiemolloy @EricWittmann @jenny-s51 @mfrances17 Addressed review comments. Let me know if there's anything else. Thanks.

EricWittmann
EricWittmann previously approved these changes Mar 30, 2020
mfrances17
mfrances17 previously approved these changes Mar 30, 2020
jenny-s51
jenny-s51 previously approved these changes Mar 30, 2020
Copy link
Contributor

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

LGTM @dlabaj! Working as expected now.

@christiemolloy
Copy link
Contributor

Drawer is broken

Screen Shot 2020-03-30 at 11 09 33 AM

Screen Shot 2020-03-30 at 11 11 20 AM

padding: 0 !important;
}

.ap--drawer-panel-body .pf-c-drawer__panel-body {
Copy link
Contributor

Choose a reason for hiding this comment

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

incorrect spelling this is why the padding is there

@dlabaj dlabaj dismissed stale reviews from jenny-s51, mfrances17, and EricWittmann via 93c20a9 March 30, 2020 18:15
Copy link
Contributor

@christiemolloy christiemolloy left a comment

Choose a reason for hiding this comment

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

thanks

@mfrances17 mfrances17 merged commit 6e142ed into Apicurio:master Mar 30, 2020
@dlabaj dlabaj deleted the fix/cleanup branch April 11, 2020 14:21
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.

5 participants