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

Jeff/misc from sinpe branch #254

Merged
merged 5 commits into from
Jan 28, 2025
Merged

Jeff/misc from sinpe branch #254

merged 5 commits into from
Jan 28, 2025

Conversation

jeffesquivels
Copy link
Member

@jeffesquivels jeffesquivels commented Jan 22, 2025

More spin off commits from the sinpe branch.

The ones here are based on da0d25b, 8d9379a, 7a109ee and 663f583 without the sinpe movil specific parts.

@jeffesquivels jeffesquivels self-assigned this Jan 22, 2025
@sisou
Copy link
Member

sisou commented Jan 23, 2025

@onmax Can you explain why the RouteName enum is useful? It doesn't improve the situation in this PR I feel.

@onmax
Copy link
Member

onmax commented Jan 23, 2025

I introduced this because I often misspelled route names in the past. With TS, improve DX in both dev and building. However, I can undo it if you don't think it's worth it.

In code looks like:

router.push({ name: 'my-modal' }); // before
router.push({ name: RouteName.MyModal }); // after

plus, this change does not break anything

PS: Similar principle that you have commented before in one of my other PR

@sisou
Copy link
Member

sisou commented Jan 24, 2025

Well, for now you just moved the string definition from inside the route object to outside. But are the route names also going to be replaced in other places? Only then it makes sense.

@onmax
Copy link
Member

onmax commented Jan 24, 2025

See #255

@jeffesquivels
Copy link
Member Author

jeffesquivels commented Jan 24, 2025

Ah sorry, that's probably a mistake on my part, I didn't realized there was a commit where everything else got changed (I thought this was just going to be a partial migration kind of thing and then route names in other places would get changed whenever that code got changed in the future).

onmax and others added 5 commits January 27, 2025 17:00
Authored-By: Maxi <maximogarciamtnez@gmail.com>
This is not a breaking change, and it will help us ensure the string values are consistent across the app

Authored-By: Maxi <maximogarciamtnez@gmail.com>
Authored-By: Maxi <maximogarciamtnez@gmail.com>
@jeffesquivels jeffesquivels force-pushed the jeff/misc_from_sinpe_branch branch from 91fffb4 to d8f88d1 Compare January 27, 2025 23:01
@sisou sisou merged commit d8f88d1 into master Jan 28, 2025
1 check passed
@sisou sisou deleted the jeff/misc_from_sinpe_branch branch January 28, 2025 13:07
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.

3 participants