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

Pro 6974 update UI fields #4836

Merged
merged 18 commits into from
Jan 22, 2025
Merged

Pro 6974 update UI fields #4836

merged 18 commits into from
Jan 22, 2025

Conversation

ValJed
Copy link
Contributor

@ValJed ValJed commented Jan 15, 2025

PRO-6974

Summary

See tickets, also tries to improve some inline generic style.

What are the specific steps to test this change?

See design, to test with this palette PR

Cypress tests 🟢

What kind of change does this PR introduce?

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

Make sure the PR fulfills these requirements:

  • It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
  • The changelog is updated
  • Related documentation has been updated
  • Related tests have been updated

Copy link

linear bot commented Jan 15, 2025

@ValJed ValJed marked this pull request as draft January 15, 2025 15:52
@ValJed ValJed self-assigned this Jan 15, 2025
@@ -108,7 +155,7 @@ export default {
// adapted from http://danielstern.ca/range.css/#/
.apos-range__input {
width: 100%;
margin: 5px 0;
margin: 5px 0; // Question: Shoud we keep that margin? It creates more space than for other fields.
Copy link
Contributor Author

@ValJed ValJed Jan 15, 2025

Choose a reason for hiding this comment

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

@stuartromanek
Question: Should we keep this margin? It creates more space than for other fields.

@@ -190,6 +192,10 @@ export default {
}
}

.apos-field__label-meta:empty {
display: none;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removes meta span container if it's empty (avoid to screwing up display with new range info part)

.apos-field__info,
.apos-input-wrapper {
width: 48%;
}
Copy link
Contributor Author

@ValJed ValJed Jan 15, 2025

Choose a reason for hiding this comment

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

Not sure it's a good thing to remove totally this. Or maybe we can add a flex-shrink: 1.

edit: This is what I did below, feel free to test it, this margin could be of another value of course

@ValJed ValJed requested a review from stuartromanek January 15, 2025 17:06
@ValJed ValJed marked this pull request as ready for review January 15, 2025 17:06
@ValJed ValJed force-pushed the pro-6974-update-ui-fields branch from 0bb9728 to a29b735 Compare January 16, 2025 16:48
Copy link
Member

@stuartromanek stuartromanek left a comment

Choose a reason for hiding this comment

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

@ValJed ValJed requested a review from stuartromanek January 21, 2025 14:35
@@ -52,9 +55,6 @@ export default {
}
}
return false;
},
convert(value) {
return parseFloat(value);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed anymore since we ask vue to set the value of the range input as a number directly.

@@ -5,10 +5,14 @@ export default {
mixins: [ AposInputMixin ],
data() {
return {
next: this.initNext(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid setting it to a string in AposInputMixin before to transform it

@@ -21,27 +25,26 @@ export default {
isSet() {
// Detect whether or not a range is currently unset
// Use this flag to hide/show UI elements
if (this.next >= this.field.min) {
if (typeof this.next === 'number' && this.next >= this.field.min) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must be a number all the time now since we added the number modifier to range input

}

return this.getDefault();
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must be a number all the time now since we added the number modifier to range input

}

.apos-input-wrapper {
flex-grow: 1;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improve label and field positions in inline

@ValJed ValJed force-pushed the pro-6974-update-ui-fields branch from ffbae39 to cd19f50 Compare January 21, 2025 17:16
stuartromanek
stuartromanek previously approved these changes Jan 21, 2025
stuartromanek
stuartromanek previously approved these changes Jan 22, 2025
@ValJed ValJed merged commit 7a8c136 into main Jan 22, 2025
9 checks passed
@ValJed ValJed deleted the pro-6974-update-ui-fields branch January 22, 2025 15:27
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.

2 participants