Skip to content

Conversation

@AllanOXDi
Copy link
Member

Summary

This PR trims whitespace on all form fields for all side panels

Before

Screen.Recording.2025-07-30.at.21.21.07.mov

After

Screen.Recording.2025-07-30.at.21.16.08.mov

References

#13562

Reviewer guidance

Go to Facility > Classes and try to copy a class by entering a name with empty space at the beginning or the end

@github-actions github-actions bot added APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend SIZE: small labels Jul 30, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jul 30, 2025

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

This looks like it'll get the job done, but Vue provides a big of magic w/ the v-model.trim modifier - so you could just add .trim to where v-model is used.

facility: activeFacilityId.value,
username: username.value,
full_name: fullName.value,
full_name: fullName.value.trim(),
Copy link
Member Author

Choose a reason for hiding this comment

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

@nucleogenesis I think we can keep the current approach of trimming in the data object here?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that works - it looks like reworking the v-model here to use trim might be a bit trickier since the value props are set directly w/ the .sync.

Might be worth trying adding .trim to the prop like:

<FullNameTextbox
    ref="fullNameTextbox"
    // ... other props ...
    :value.sync.trim="fullName"    // note the .trim here
    // ... other props ...
  />

Although, that may only be available using v-model.

In any case, no need to spend much time on that beyond seeing what you learn. Keeping the .trim() here is 👍

Copy link
Member Author

@AllanOXDi AllanOXDi Jul 31, 2025

Choose a reason for hiding this comment

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

I actually there is no need to do so - its already being handled.

@pcenov
Copy link
Member

pcenov commented Aug 4, 2025

Hi @AllanOXDi - not sure how likely is this to happen in a real world user interaction but during testing I noticed that if I leave empty space between the words of the class name, then it's possible to create seemingly duplicate class names. This happens both when creating and copying a class.

Creating a new class:

sp-mdl.mp4

Copying a class:

copy.class.mp4

Other than that, I confirm that the issue reported in #13562 is fixed now.

@AllanOXDi
Copy link
Member Author

@marcellamaki How could we handle this, can we make the more space created be visible in the list of class names or we can treat the name as the same even when a user adds more space between the words.

@marcellamaki
Copy link
Member

we can treat the name as the same even when a user adds more space between the words

I think this option makes more sense to me. We can compare the "trimmed" versions of the names to see if it already exists, rather than what was entered by the user (with extra spaces). What do you think about that?

@AllanOXDi AllanOXDi force-pushed the remove-white-spaces-from-formfields branch from cdb4eef to e4ca623 Compare August 13, 2025 13:19
@nucleogenesis
Copy link
Member

@AllanOXDi @marcellamaki

We could opt to show the user the whitespace they entered by making a CSS change to white-space: pre (I think this would require /deep/ selectors) - and when I tried it in my browser it borked the width of the column so we'd also have to restrict the width somehow. This is probably more trouble than it's worth - I may be wrong but I suspect users aren't typically using multiple spaces in their class names.

So it'd probably be safe to collapse multiple spaces into one space, then trim it (ie, A BB CCC DDDD -> A BB CCC DDDD) where the user enters the new name.

@nucleogenesis nucleogenesis self-assigned this Aug 13, 2025
@AllanOXDi
Copy link
Member Author

I think we are just doing that in the current state.

@nucleogenesis
Copy link
Member

nucleogenesis commented Aug 19, 2025

Hrm - there's some weirdness to this that makes me unsure about how to proceed.

One issue is that I think if we're going to introduce client-side validations that aren't backed by the DB model we should be sure to apply that validation to that particular data (in our case a Classroom and it's name) across all places where it can be edited. (Not a huge deal).

But then there is an issue in validating strings after applying the space-collapsing:

2025-08-19.13-10-22.mp4

See how no matter how many spaces there are between words - if all of the words are the same and in the same order, they're treated as the same string. I guess this isn't really the biggest issue, but it's a bit odd to do this but then let people enter too many spaces.


That all said, I'm liable to be overthinking this altogether as it is a bit of an edge case. I'll follow-up w/ a few others to get some thoughts before merging.

@nucleogenesis
Copy link
Member

@AllanOXDi noting here that once #13665 merges you'll need to be sure that your space-reducing function is applied to the class name throughout (ie, the error message for duplicate class names).

@AllanOXDi AllanOXDi force-pushed the remove-white-spaces-from-formfields branch from e4ca623 to 0285424 Compare August 27, 2025 16:38
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Sweet - tested it out locally and things looked right as expected. I tested case & spaces between words and the validation was all correct.

@AllanOXDi AllanOXDi merged commit e93ebf1 into learningequality:develop Aug 28, 2025
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend SIZE: small SIZE: very small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants