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

Removing texts part d#161 #286

Merged
merged 9 commits into from
Dec 11, 2023
Merged

Removing texts part d#161 #286

merged 9 commits into from
Dec 11, 2023

Conversation

amabelleS
Copy link
Collaborator

Description

Removed from: TimeSelector, StopSelector, RouteSelector, OperatorSelector, MinuteSelector, LineSelector and Profile.

Next, maybe in this PR, I want to remove texts from BusToolTip. but I'm not sure where to find it in the UI, and I need to check because some translations are missing there even in the texts file...

@amabelleS amabelleS requested a review from NoamGaash as a code owner December 10, 2023 00:38
Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

👏 👏 👏

@@ -16,6 +17,7 @@ export function BusToolTip({ position, icon }: BusToolTipProps) {
const [siriRide, setSiriRide] = useState<SiriRideWithRelatedPydanticModel | undefined>()
const [isLoading, setIsLoading] = useState(false)
const [showJson, setShowJson] = useState(false)
const { t } = useTranslation()
Copy link
Member

Choose a reason for hiding this comment

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

this one can be removed

@NoamGaash
Copy link
Member

the bus tooltip can be found in every map, by clicking a bus icon 🚌
thanks for your contributions!

@shootermv
Copy link
Contributor

shootermv commented Dec 10, 2023

the only problem now - is unused t variable at BusTooltip.tsx file

@amabelleS
Copy link
Collaborator Author

the only problem now - is unused t variable at BusTooltip.tsx file

That's because I want to push it to this PR and not the next one:) that is, to remove TEXTS from BusTooltip :)

@NoamGaash
Copy link
Member

so please remove this t for now, and re-add it in the next PR -
the reasoning behind it is that we want to keep things "clean", without things that are in progress. it can confuse others

@amabelleS
Copy link
Collaborator Author

the bus tooltip can be found in every map, by clicking a bus icon 🚌 thanks for your contributions!

Like this one:
image

But on hover, there is no tooltip, and I can not see all the information on the bus, in the UI... also, there are the missing translations from the he/en.json: line, time, destination, and more.
So I am changing code, adding values, all of that without any way to check:(

Anyway, there's a new push to this PR, I removed TEXTS from BusToolTip.

@amabelleS amabelleS requested a review from NoamGaash December 10, 2023 21:05
@amabelleS
Copy link
Collaborator Author

so please remove this t for now, and re-add it in the next PR - the reasoning behind it is that we want to keep things "clean", without things that are in progress. it can confuse others

Sorry, I saw this comment just now, after I made the changes.

Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

Perfect!
👏

@NoamGaash
Copy link
Member

@amabelleS
Try clicking one of the busses:
image

@shootermv shootermv merged commit b5b8a0d into main Dec 11, 2023
9 checks passed
@shootermv shootermv deleted the removing-texts_partD#161 branch December 11, 2023 11: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