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

Simplify css and DropzoneBetweenElements #3158

Conversation

Kadrian
Copy link
Collaborator

@Kadrian Kadrian commented Aug 28, 2023

No description provided.


const Line = styled("div")`
background-color: ${({ theme }) => theme.col.blueGrayDark};
margin: 1px 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need for margin or flex-grow, instead just setting width: 100%

height: number;
}

const Root = styled("div")`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need for a root wrapper

@@ -5,52 +5,41 @@ import Dropzone, {
PossibleDroppableObject,
} from "../../ui-components/Dropzone";

interface Props {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Co-locate props with where they're used below

position: absolute;
top: 0;
left: 0;
transform: translateY(calc(-50% - ${LINE_HEIGHT / 2}px));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to allow passing height or top, we can simply set height to 30px, that makes the droppable area always 30px high. Then we can use transform to move the dropzone "between" the elements by -50% related to its own height of 30px, minus half the height of the line. That makes it sit right in the middle

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thats a nice solution for most of the Elements. The issue I see now however, is that the Dropzone at the bottom will in part be overshadowed by the last DropzoneBetweenElements.
image

Do you think thats an issue? I think it could be annoying for the users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah good point, fixed it with a classname and making the last dropzone less high

@Kadrian
Copy link
Collaborator Author

Kadrian commented Aug 28, 2023

Hey @fabian-bizfactory , I thought this was quicker than doing another round of review

Copy link
Collaborator

@fabian-bizfactory fabian-bizfactory left a comment

Choose a reason for hiding this comment

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

Those changes would prevent the overlap.

@Kadrian Kadrian merged commit 028a378 into feature/allow-insertion-of-concepts-between-each-other Aug 28, 2023
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the simplify-dropzone-between-elements branch August 28, 2023 12:40
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.

None yet

2 participants