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

Datepicker 0 #132

Merged
merged 10 commits into from
Jul 15, 2019
Merged

Datepicker 0 #132

merged 10 commits into from
Jul 15, 2019

Conversation

lfbergee
Copy link
Contributor

@lfbergee lfbergee commented Jul 10, 2019

📥 Proposed changes

Add initial package and design for datepicker

☑️ Submission checklist

  • I have read the CONTRIBUTING doc
  • yarn build works locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

💬 Further comments

Notes:

  • Pushes content down when open, not over other content, don't know if we want that
  • Uses fafafa as background to hide input field background, its using fff with fff as background in inputfield, however that do not exsist in input yet. Should we add?
  • dropdown not implemented as API might change a lot with open PR

closes: #46

Leiv Fredrik Berge added 3 commits July 10, 2019 14:18
affects: @fremtind/jkl-datepicker-react, @fremtind/jkl-text-input-react
affects: @fremtind/jkl-datepicker-react, @fremtind/jkl-datepicker
@lfbergee lfbergee self-assigned this Jul 10, 2019
@lfbergee
Copy link
Contributor Author

Screenshot from 2019-07-10 15-53-52
Screenshot from 2019-07-10 15-54-09

@lfbergee
Copy link
Contributor Author

I'm having some issues writing tests that actually change the date. getBytText() seems to create an eternal loop that crashes the js-runtime.

@piofinn piofinn self-requested a review July 11, 2019 14:42
@piofinn
Copy link
Contributor

piofinn commented Jul 12, 2019

I think there are some issues with testing web components out of the box with jest/testing library. @Saegrov probably knows more about this, though.

Copy link
Contributor

@piofinn piofinn left a comment

Choose a reason for hiding this comment

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

Nice implementation! The comments below about double-imprted styles is something we should look at in all packages.

@import "~@fremtind/jkl-core/build/scss/variables/colors.scss";
@import "~@fremtind/jkl-core/build/scss/typography/paragraphs.scss";
@import "~@fremtind/jkl-core/build/scss/functions.scss";
@import "~@fremtind/jkl-text-input/text-input.scss";
Copy link
Contributor

Choose a reason for hiding this comment

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

We should avoid importing other stylesheets directly, as this will duplicate the rules (presuming text-input is used elsewhere).

}

caption {
@extend .jkl-p;
Copy link
Contributor

Choose a reason for hiding this comment

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

As above. This will include the style rules for paragraphs in duplicate for this component (since we import paragraph.scss). Optimally we would just give the element the .jkl-p class, but I guess that is not an option here. I still think we should try to get rid of the duplicate. Maybe make a paragraph @mixin instead?

@piofinn piofinn self-requested a review July 15, 2019 08:07
@lfbergee lfbergee merged commit ebac39a into master Jul 15, 2019
@lfbergee lfbergee deleted the datepicker-0 branch July 15, 2019 08:07
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.

Components: Datepicker
2 participants