Skip to content

Conversation

@KaiVandivier
Copy link
Contributor

Adds support for adding native min, max and step attributes to inputs in Input and InputField

Relates to this discussion: dhis2/notes#240

@cypress
Copy link

cypress bot commented Feb 24, 2021



Test summary

501 0 0 0


Run details

Project ui
Status Passed
Commit 8e7e25f
Started Feb 25, 2021 11:56 AM
Ended Feb 25, 2021 12:00 PM
Duration 03:45 💡
OS Linux Ubuntu - 20.04
Browser Electron 87

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@KaiVandivier KaiVandivier requested a review from a user February 24, 2021 13:53
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Changes look good to me, only thing that I'm not sure about is the accepted type (see comment). Once that's resolved this should be good to merge 👍

@ghost
Copy link

ghost commented Feb 24, 2021

Ah, and this: I think I'd personally classify this update as a feat update, unless I'm missing something it seems like we didn't support this before and it's a new feature. But if I'm misinterpreting that and fix is actually better that's fine with me as well.

One other thing: it's good to specify a scope for all commits. So instead of say fix: ..., do fix(input): .... That way the changelog changes will be grouped by topic and easier to read. Also, I'd get rid of the merge commit by rebasing on master, that way you'll have a cleaner history before merging (not sure if that'll have an effect for semantic release, but it seems cleaner to me).

@KaiVandivier
Copy link
Contributor Author

Ah, and this: I think I'd personally classify this update as a feat update, unless I'm missing something it seems like we didn't support this before and it's a new feature. But if I'm misinterpreting that and fix is actually better that's fine with me as well.

'feat' is fine with me 👍

One other thing: it's good to specify a scope for all commits. So instead of say fix: ..., do fix(input): .... That way the changelog changes will be grouped by topic and easier to read.

Ah, that sounds good - I'll do that from now on. I didn't do it for that commit above because it includes Input and InputField, but maybe it would have been better to split them up for scoping purposes.

Also, I'd get rid of the merge commit by rebasing on master, that way you'll have a cleaner history before merging (not sure if that'll have an effect for semantic release, but it seems cleaner to me).

I'm planning to squash merge because I don't think the individual commits here are important to see on master, does that seem fine?

Thanks @ismay!

@KaiVandivier KaiVandivier changed the title fix(input): add min/max/step attr for inputs (LIBS-133) feat(input): add min/max/step attr for inputs (LIBS-133) Feb 24, 2021
@KaiVandivier KaiVandivier force-pushed the fix/add-min-max-step-attr-for-inputs branch from e46ca5c to d2c3123 Compare February 25, 2021 10:40
@KaiVandivier
Copy link
Contributor Author

Cleaned up history to two commits with correct scopes ^

@KaiVandivier KaiVandivier force-pushed the fix/add-min-max-step-attr-for-inputs branch from d2c3123 to 8e7e25f Compare February 25, 2021 11:52
@ghost ghost self-requested a review February 25, 2021 12:03
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good, nice work! I'd recommend a regular merge over a squash merge in this case, since you have two updates in the PR. Would be a shame to let the rebasing go to waste 😋

@KaiVandivier KaiVandivier merged commit de95bd7 into master Feb 25, 2021
@KaiVandivier KaiVandivier deleted the fix/add-min-max-step-attr-for-inputs branch February 25, 2021 12:15
@dhis2-bot
Copy link
Contributor

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.

5 participants