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

feat: add number as field #849

Merged
merged 5 commits into from
Nov 21, 2024
Merged

feat: add number as field #849

merged 5 commits into from
Nov 21, 2024

Conversation

ErikSin
Copy link
Contributor

@ErikSin ErikSin commented Nov 18, 2024

closes #836

@ErikSin ErikSin requested a review from cimigree November 18, 2024 12:53
Copy link
Contributor

@cimigree cimigree left a comment

Choose a reason for hiding this comment

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

I am sure this will be a great addition! I have a few minor questions and comments.


interface Props {
field: Field;
}

export const QuestionLabel = ({field}: Props) => {
const hint = <FormattedFieldProp field={field} propName="placeholder" />;
// const hint = <FormattedFieldProp field={field} propName="placeholder" />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the commented code and console log have a purpose here? If not, can you please remove or adjust?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

import {Field} from '@comapeo/schema';
import {HeaderText} from '../../sharedComponents/Text/HeaderText';
import {Header} from 'react-native/Libraries/NewAppScreen';
Copy link
Contributor

Choose a reason for hiding this comment

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

The import for Header is not being used/ is a mistake probably

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const {updateTags} = useDraftObservation();
const value = tags ? tags[field.tagKey] : '';
return (
<React.Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be/ can there be a minimum and/ or maximum? And is it intended that negative numbers and decimals aren't allowed?

@@ -22,5 +23,9 @@ export const Question = ({field}: QuestionProps) => {
return <SelectMultiple field={field as SelectMultipleField} />;
}

if (field.type === 'number') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a map to be a little more scalable/ elegant? Something like

const FIELD_COMPONENTS: Record<string, React.ComponentType<{ field: Field }>> = {
  selectOne: SelectOne,
  selectMultiple: SelectMultiple,
  text: TextArea,
  number: Number,
};

export const Question = ({ field }: QuestionProps) => {
  const FieldComponent = FIELD_COMPONENTS[field.type] || TextArea;
  return <FieldComponent field={field} />;
};

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 tried this but there are some annoying typing issues that arise. the field prop for each of these components are actually slightly modified subsets of Field in schema. This is because the React Component property has certain properties that are necessary for the UI but not necessary for the actual schema.

I tried widening the definition of React.ComponentType<{ field: Field }>> to include the other types of props, but that lead into a TS battle of narrowing props that ended up making the mapping still complicated

@ErikSin ErikSin requested a review from cimigree November 19, 2024 11:38
Copy link
Contributor

@cimigree cimigree left a comment

Choose a reason for hiding this comment

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

Do you think it's ok that numbers like this are allowed? Should we just trust the observers?

@ErikSin
Copy link
Contributor Author

ErikSin commented Nov 21, 2024

Do you think it's ok that numbers like this are allowed? Should we just trust the observers?

Its fixed now!

@ErikSin ErikSin requested a review from cimigree November 21, 2024 10:58
@@ -20,7 +20,8 @@ export const Number = React.memo<{field: Field}>(({field}) => {
field.tagKey,
newVal
.replace(/[^0-9.-]/g, '') // Allow digits, decimal, and negative sign
.replace(/(?!^)-/g, ''), // Remove any minus sign that is not at the start
.replace(/(?!^)-/g, '') // Remove any minus sign that is not at the start
Copy link
Contributor

Choose a reason for hiding this comment

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

This works, but you might consider .replace(/^(-?)(\d*)(\.?\d*).*/, '$1$2$3') instead to have one line for the regex. It does the same thing but in one line instead of two...

@ErikSin ErikSin merged commit 001a75e into develop Nov 21, 2024
3 checks passed
@ErikSin ErikSin deleted the number-field branch November 21, 2024 18:08
ErikSin added a commit that referenced this pull request Nov 25, 2024
* feat: add number as field

* chore: remove unnecessary code

* chore: allow negative and decimals

* chore: returned Question Component to original state

* chore: only allow one decimal
ErikSin added a commit that referenced this pull request Dec 17, 2024
* Release v1.1.0

* fix: observations disappearing when tracking starts (#837) (#838)

* fix: Bottom Sheet Opens when animations disabled. (#840) (#841)

* fix: drawer not closing when animations disabled (#842)

* fix: microphone permission modal has correct text (#843)

* Makes sure text in permissions modal is correct. Removes some old console logs.

* fix: delete map shows confirmation twice (#845)

* Adds on success handler to the hook so that the modal is only closed after the refresh is done.

* Adds body text and header text to the background map files so that they line up with figma designs. Adds warning red so that the red color is as design specifies

* Uses Full screen menu list for map management menu screen.

* Fix: share button sometimes not working after first share (#847)

* Fetches a new url for attachments each time the share button is pressed. Uses new BodyText component instead of text.

* chore: remove unused useClearAllPendingInvites hook (#852)

* Removes audio files from sharing an observation. Removes type check that is no longer needed. (#854)

* fix: simplify technical implementation of importing custom map file (#855)

* feat: add number as field (#849)

* feat: add number as field

* chore: remove unnecessary code

* chore: allow negative and decimals

* chore: returned Question Component to original state

* chore: only allow one decimal

* fix: project leaving inconsistencies (#853)

* Uses a state variable and use effect in the bottom sheet modal related to project invitation so the success modal shows properly.

* Fixes crash when permission is taken away. (#851)

* chore: Implement type ramp (#818)

* chore: added rubik and updated text component to use it

* chore: update header

* chore: update description

* chore: update button to use rubik

* chore: update drawer header to rubik

* chore: update text to use variant flag

* chore: update some headers

* chore: revert text and add deprecated tag

* chore: remove all unneccesary fonts

* chore: create header component

* chore: created body text

* chore: update jsdoc comments

* chore:update bodyText to not have line height

* chore: update data and privacy screen to use new text components

* chore:update buttons

* chore: update invite screen

* chore: invite accpeted

* chore: waiting for invite to accept screen

* chore: update drawer text

* chore: text align

* chore: update sync screen

* chore: usefontSize map

* chore: type 24 converted to 14

* chore: update inputs (#830)

* chore: create checkbox using svgs

* chore: update metrics to use new checkbox

* chore: update input color and font

* fix: audio permission modal spacing hiding button (#865) (#873)

* Fixes full sheet on bottom sheet modal back

* Undoes last change. Takes out padding so permission audio fits.

Co-authored-by: cimigree <[email protected]>

* Adds snappoints prop to bottom sheet modal to help sizing on full screen modals. (#878) (#889)

Co-authored-by: cimigree <[email protected]>

* feat: list tracks in observation (#890)

* feat: show linked track in observation (#839)

* chore: create reuseable accordian

* chore: reuseable track

* chore: remove component

* chore: plural of observation

* chore:translations

* chore: create track list

* chore: translations

* chore: fix styling

* chore: update text to use customtext

* chore: rename track list to track accordian

* chore: change nav to popTo from navigate

* chore: remove flex:1

* chore: refactor track list in observation with maintainable architecture (#883)

* chore: find associated track with test

* chore: refactor TrackAccordian

* chore: integrate new tracsk accordian

* chore: pr review

* chore: change to mts file

* chore: remove mock data and testing for ci

---------

Co-authored-by: cimigree <[email protected]>
Co-authored-by: Andrew Chou <[email protected]>
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.

Number field needed for configuration questions
2 participants