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.

Sign up for GitHub

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

Jump to bottom

DatePicker and DatePickerInput implementation duplicates too many classes #2822

Open
klaascuvelier opened this issue Mar 18, 2024 · 3 comments · May be fixed by #2823
Open

DatePicker and DatePickerInput implementation duplicates too many classes #2822

klaascuvelier opened this issue Mar 18, 2024 · 3 comments · May be fixed by #2823

Comments

@klaascuvelier
Copy link
Contributor

Describe in detail the issue you're having.

I noticed that the implementation for datepicker.component.ts and datepicker-input.component.ts have quite some duplication in the template.

This results in the DOM with a structure like this

<!-- start datepicker.component.ts -->
<div class="cds--form-item">
  <div class="cds--date-picker">
    <div class="cds--date-picker-container">
      <!-- start datepicker-input.component.ts -->
      <div class="cds--form-item">
        <div class="cds--date-picker">
          <div class="cds--date-picker-container">
            <label>...</label>
            <div class="cds--date-picker-input__wrapper>
              <span>
                <input />
              </span>
            </div>
          </div>
        </div>   
      </div>
      <!-- end datepicker-input.component.ts -->
    </div>
  </div>
</div>
<!-- end datepicker.component.ts -->

This is a pretty bloated DOM structure and can be simplified a lot. The react implementation is a lot simpler and has not class duplication.

I created this table as comparison, hope this works to show the problem:

component angular react
datepicker .cds--form-item > .cds--date-picker > .cds--date-picker--container .cds--form-item > .cds--date-picker
datepicker-input .cds--form-item > .cds--date-picker > .cds--date-picker--container .cds--date-picker--container
@klaascuvelier klaascuvelier changed the title DatePicker and DatePickerInput share too many elements/classes DatePicker and DatePickerInput implementation duplicates too many classes Mar 18, 2024
@klaascuvelier
Copy link
Contributor Author

klaascuvelier commented Mar 18, 2024 •
edited

I just found out possible 2 reasons why it might be implemented this way:

  • the Angular datepicker component does not support simple as a type, instead the datepicker-input is used directly. In the React implement the datepicker-input is not used directly but only through datepicker.
    Changing this implementation to align with React and to remove the duplication would result in a breaking change.
  • When having a range type and having the cds--date-picker--container class inside the datepicker-input there are no 2 elements with cds--date-picker--container next to eachother in the DOM, so the carbon/styles for aligning 2 inputs next to each other don't apply (because of the wrapping cds-datepicker-input element that wraps it). This could be fixed however by adding the class to through hostbindings

@klaascuvelier klaascuvelier linked a pull request Mar 18, 2024 that will close this issue
Open
@Akshat55
Copy link
Contributor

I agree with you on this! The structure is bloated and the change is a breaking change. I'm thinking it might make sense to make the PR target the next branch where we can release a rc version and release it officially with v12 next year. 🤔

@klaascuvelier
Copy link
Contributor Author

klaascuvelier commented Mar 21, 2024 •
edited

Hi @Akshat55 thanks for the feedback. I agree that maybe targetting a later release is fine. For us this is not a blocking issue, just a nice improvement :)
Let me know if there's something more I can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[breaking] refactor: Datepicker simple/range/single klaascuvelier/carbon-components-angular
2 participants
@klaascuvelier @Akshat55