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.

Sign up for GitHub

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

fix hyphen issue for sync and controller code #1725

Merged
merged 2 commits into from
Aug 21, 2024
Merged

fix hyphen issue for sync and controller code #1725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ceb6dec
fix hyphen issue for sync and controller code
yadvirkaur Jul 24, 2024
14df762
minnor change for sync and controller code
yadvirkaur Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/js/pages/Sync/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,13 @@ class Sync extends React.PureComponent {
onKeyUp={e => {
const typedValue = e.currentTarget.value;
const typedChar = e.key;
const hasHyphen = typedValue.includes('-');
const parsedValue = typedValue.match('^[A-Z,a-z]{3}');
const d = parsedValue ? parsedValue[0] === typedValue : false;
if (d && typedChar !== 'Backspace') {
e.currentTarget.value = typedValue + '-';
} else if (typedChar === '-' && hasHyphen) {
e.currentTarget.value = typedValue.replace(/[-]+/g, '-');
}
}}
/>
Expand Down
31 changes: 11 additions & 20 deletions src/js/pages/WebController/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { Stub } from '../Search/Layout';
import ControllerShabad from '@/pages/WebController/shabad';
import { versesToGurbani } from '@/util';
import ShabadControls from '@/components/ShabadControls';
import { isMobile } from 'react-device-detect';
export default class WebControllerPage extends React.PureComponent {
constructor(props) {
super(props);
Expand Down Expand Up @@ -274,26 +273,18 @@ export default class WebControllerPage extends React.PureComponent {
pattern="[A-Z,a-z]{3}-[A-Z,a-z]{3}"
onKeyUp={e => {
const typedValue = e.currentTarget.value;
if (isMobile) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@yadvirkaur I can see there is difference in the logic in Mobile and Desktop. Is it something reconsidered now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously:

For the Bani Controller:

  • Mobile Devices: The code checked if the user had typed four characters without a hyphen and then inserted a hyphen after the third character.

  • Other Devices: The code added a hyphen automatically after the third character.

For Sangat Sync:

The code for all devices added a hyphen automatically after the third character.

Now:
I've now unified the logic for the Bani Controller to ensure consistent behaviour across all devices. The code now adds a hyphen automatically after the third character for all devices and ensures that no extra hyphens are added.

Copy link
Collaborator

@gurjit03 gurjit03 Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@yadvirkaur cool, thanks for the explainations, I totally got it. It's just I am unaware of these changes, if these are the changes requested lets do it. Also I would add, lets please make sure all these changes are mentioned in the ticket too, easier to review and followup.
Otherwise I would request to kindly confirm, as I am sure there might be good reason for this seperation.

Copy link
Collaborator Author

@yadvirkaur yadvirkaur Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@gurjit03 hnji veerji!
The mobile-specific logic for the Bani controller is fine as it is since it adds a hyphen after 4 characters only if the user hasn't already added one, so no risk of double hyphens there.

However, this can cause confusion between the Bani Controller and Sangat Sync since they handle hyphens differently for mobile devices.

@saintsoldierx @Gauravjeetsingh veerji please confirm if we need separation of logic or if we can have a unified logic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't remember the reason for separate logic, but if its working with the unified logic, let's keep that. Realistically it makes sense to have same logic for both mobile and desktop browsers.

const parsedValue = typedValue.match('^[A-Z,a-z]{4}');
const isParsedValueExist = parsedValue
? parsedValue[0] === typedValue
: false;
if (isParsedValueExist) {
const lastChar = typedValue.slice(-1);
e.currentTarget.value =
typedValue.slice(0, 3) + '-' + lastChar;
}
} else {
const typedChar = e.key;
const parsedValue = typedValue.match('^[A-Z,a-z]{3}');
const isParsedValueExist = parsedValue
? parsedValue[0] === typedValue
: false;
if (isParsedValueExist && typedChar !== 'Backspace') {
e.currentTarget.value = typedValue + '-';
}
const typedChar = e.key;
const hasHyphen = typedValue.includes('-');
const parsedValue = typedValue.match('^[A-Z,a-z]{3}');
const isParsedValueExist = parsedValue
? parsedValue[0] === typedValue
: false;
if (isParsedValueExist && typedChar !== 'Backspace') {
e.currentTarget.value = typedValue + '-';
} else if (typedChar === '-' && hasHyphen) {
e.currentTarget.value = typedValue.replace(/[-]+/g, '-');
}

}}
required
/>
Expand Down