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

Dropdown Component + some general adjustments #21

Merged
merged 9 commits into from
Dec 18, 2023
Merged

Dropdown Component + some general adjustments #21

merged 9 commits into from
Dec 18, 2023

Conversation

nicosammito
Copy link
Contributor

No description provided.

@nicosammito nicosammito added the enhancement New feature or request label Dec 18, 2023
@nicosammito nicosammito self-assigned this Dec 18, 2023
Copy link
Contributor

github-actions bot commented Dec 18, 2023

GitLab Pipeline Action

General information

Link to pipeline: https://gitlab.com/code0-tech/base-ui/-/pipelines/1110814035

Status: Passed
Duration: 2 minutes

Job summaries

storybook:build

Storybook available at https://code0-tech.gitlab.io/-/base-ui/-/jobs/5772652264/artifacts/storybook-static/index.html

@nicosammito
Copy link
Contributor Author

image
Potential bug either with storybook deployment or we need to find another way to solve the height problem inside the ButtonGroup. Locally it works fine with display flex on children. On production, it seems not to work, for approving this MR its not mandatory, but we need to track this.

@nicosammito
Copy link
Contributor Author

image Potential bug either with storybook deployment or we need to find another way to solve the height problem inside the ButtonGroup. Locally it works fine with display flex on children. On production, it seems not to work, for approving this MR its not mandatory, but we need to track this.

image
seems like that its not because of the display flex not correctly applied but because of the margin-bottom from the dropdown

@nicosammito
Copy link
Contributor Author

image Potential bug either with storybook deployment or we need to find another way to solve the height problem inside the ButtonGroup. Locally it works fine with display flex on children. On production, it seems not to work, for approving this MR its not mandatory, but we need to track this.

image seems like that its not because of the display flex not correctly applied but because of the margin-bottom from the dropdown

Issue resolved with 568d42c

@nicosammito
Copy link
Contributor Author

I will give it a quick self review, because no one is online right now and I am the project lead anyway

@nicosammito nicosammito merged commit 212a44b into main Dec 18, 2023
1 check passed
@nicosammito nicosammito deleted the dropdown branch December 18, 2023 07:09
@nicosammito nicosammito restored the dropdown branch December 18, 2023 07:09
@nicosammito
Copy link
Contributor Author

Lets keep the branch for now, so @Taucher2003 or @raphael-goetz can give it a good review later

@@ -67,5 +65,8 @@
},
"publishConfig": {
"access": "public"
},
"dependencies": {
"rollup-plugin-visualizer": "^5.11.0"
Copy link
Member

Choose a reason for hiding this comment

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

Is this intended to be a dependency instead of a dev devependency? Based on the name, I would expect it as dev dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's a mistake. Will be changed in a future MR.

}

/**
* Component creates an item with over effect
Copy link
Member

Choose a reason for hiding this comment

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

What the hell is an "over effect"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a typo, will be fixed within a future MR.

}

/**
* Component creates an item with over effect
Copy link
Member

Choose a reason for hiding this comment

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

Same here, what is an "over effect"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a typo, will be fixed within a future MR.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of having a utils.ts inside a utils folder, I think this should be split for better readability and easier understanding.

I think this could be split into utils/react_nodes.ts and utils/positions.ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is already merged, I will convert your ideas into issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants