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

DSP-623 Color Value Polish #191

Merged
merged 12 commits into from
Oct 5, 2020
Merged

Conversation

mdelez
Copy link
Contributor

@mdelez mdelez commented Sep 23, 2020

closes https://dasch.myjetbrains.com/youtrack/issue/DSP-623

uses dsp-js rc.11 and dsp-api rc.15

@mdelez mdelez added the styling label Sep 23, 2020
@mdelez mdelez self-assigned this Sep 23, 2020
@kilchenmann kilchenmann self-assigned this Sep 28, 2020
@kilchenmann
Copy link
Contributor

I added new style layout for read-color-value:
Screenshot 2020-09-29 at 11 28 22

And the position of the label should also be fixed in edit and create mode:
Screenshot 2020-09-29 at 12 00 33

@kilchenmann
Copy link
Contributor

@mdelez What do you think? Is it done? Can someone do the review?

@mdelez
Copy link
Contributor Author

mdelez commented Sep 30, 2020

@mdelez What do you think? Is it done? Can someone do the review?

I think the only thing I would still try to do is to center the placeholder text in the middle of the box but I know it's not very easy as I've tried as well. Other than that, it looks great! You can request a review and see what the others have to say?

@kilchenmann
Copy link
Contributor

I think the only thing I would still try to do is to center the placeholder text in the middle of the box but I know it's not very easy as I've tried as well. Other than that, it looks great! You can request a review and see what the others have to say?

Yes, I also wanted to center it. But it was not possible with my previous settings. I can try again....

@mdelez
Copy link
Contributor Author

mdelez commented Sep 30, 2020

I think the only thing I would still try to do is to center the placeholder text in the middle of the box but I know it's not very easy as I've tried as well. Other than that, it looks great! You can request a review and see what the others have to say?

Yes, I also wanted to center it. But it was not possible with my previous settings. I can try again....

It's not a big issue so don't stress too much over it :)

@kilchenmann
Copy link
Contributor

kilchenmann commented Oct 1, 2020

It's not possible to center the label, as this also affects the comment input field.

@mdelez
Copy link
Contributor Author

mdelez commented Oct 1, 2020

It's not possible to center the label, as this also affects the comment input field.

No worries :) You can request a review from Flavie since she was the one who originally created this component.

@@ -1 +1,6 @@
@import "../../../../../assets/style/viewer";

::ng-deep .mat-form-field-flex,
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of using ::ng-deep, would it be possible to do it in one of the main stylesheets (src/assets/style/*)?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I always suggested. I can move it to ds-ui.scss...

Copy link
Contributor Author

@mdelez mdelez Oct 1, 2020

Choose a reason for hiding this comment

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

If you move this to dsp-ui.scss, then this will be applied to every .mat-form-field-flex in the DOM. I think I originally tried to do this in dsp-ui.scss and select it via a class name but it wasn't working.

But this is a topic that is also not so clear to me. From the dsp-ui.scss, I expect to be able to reach any element/class in the DOM but it seems like my ability to select elements/classes inside other elements/classes is cut off at a certain point.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mdelez Yes I know. But I will use the component tag name as first selector then it should work. I'm not sure...I will try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually dealing with this same CSS scenario in this PR. I've found that within dsp-ui.scss, I can select custom class names as expected. However the issue arrises when I try to do the same thing in _viewer.scss. Do you know why I am not able to do this in _viewer.scss?

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, shouldn't we keep it consistent with the other css files? or is it something different?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mdelez Do you want to push your solution? Mine is not well done, I think...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is the decision between :host and ::ng-deep in the the color value scss file? If so, I would choose :host.

Copy link
Contributor

@kilchenmann kilchenmann Oct 1, 2020

Choose a reason for hiding this comment

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

I pushed it, but not the thing with :host the host is bigger than the input and the pointer was not only inside the box.
@mdelez You can still override my solution and/or re-request a review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aha, what you did looks good :)

Copy link
Collaborator

@flavens flavens left a comment

Choose a reason for hiding this comment

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

@kilchenmann why is there such a big gap between the color field and the comment field in edit and create modes?

Screenshot 2020-10-05 at 10 26 13

@kilchenmann
Copy link
Contributor

@kilchenmann why is there such a big gap between the color field and the comment field in edit and create modes?

Screenshot 2020-10-05 at 10 26 13

Good question. Would this be better?
Screenshot 2020-10-05 at 10 55 41

@kilchenmann
Copy link
Contributor

@kilchenmann why is there such a big gap between the color field and the comment field in edit and create modes?

@flavens: resolved in 2d9b79c

@kilchenmann kilchenmann merged commit 035c2e9 into master Oct 5, 2020
@kilchenmann kilchenmann deleted the wip/dsp-623-color-value-comp-polish branch October 5, 2020 13:31
@kilchenmann kilchenmann changed the title Color Value Polish DSP-623 Color Value Polish Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants