Feat/amenities section #486
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@pbonar do you think the orange or black icons look better in this section? |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Copilot reviewed 10 out of 15 changed files in this pull request and generated no suggestions.
Files not reviewed (5)
- lib/features/digital_guide_view/amenities/presentation/amenities_expansion_tile_content.dart: Language not supported
- lib/features/digital_guide_view/general_info/presentation/digital_guide_view.dart: Language not supported
- lib/features/digital_guide_view/general_info/presentation/widgets/headlines_section.dart: Language not supported
- lib/features/home_view/widgets/science_clubs_section.dart: Language not supported
- lib/l10n/app_pl.arb: Language not supported
This comment was marked as resolved.
This comment was marked as resolved.
|
yeah I agree, I think black icons in the section, however tbh the button on the top right should be the same color as the button on the left so that the whole top nav has the same color, but that's just my opinion |
Do you mean to change the accessibility button (with the wheelchair icon) color to orange? |
|
yeah that's what I mean, I think it would look better, we can try it out |
Okay I'll try to change and see how it looks as this button is not in the scope of any other ongoing task. Thank You <3 |
|
@mikolaj-jalocha @simon-the-shark @pbonar |
3 |
|
3 but imo the padding needs changing on all of them |
|
Option number 3 is already applied in the PR |
|
Are we ready to merge? |
| // TODO(Bartosh): url not working, nothing happens | ||
| url: | ||
| "tel:+48${digitalGuideResponseExtended.telephoneNumber.replaceAll("<p>", "").replaceAll("</p>", "")}", |
There was a problem hiding this comment.
imo we should def move the logic that extracts relevant data to repo layer and prob make it as an extension
There was a problem hiding this comment.
It was not in the scope of this task but I might try implementing it. WDYT? But I think that the existing extension for URLs might work and I don't think that another one is necessary
There was a problem hiding this comment.
I mean extension for replacing the
in the string, but u right, an overkill here. If you can please make it works and we re ready to merge then
| url: () { | ||
| final match = RegExp(r"\d{9}").firstMatch( | ||
| digitalGuideResponseExtended.telephoneNumber | ||
| .replaceAll("+48", ""), | ||
| ); | ||
| return match != null ? "tel:+48${match.group(0)}" : ""; | ||
| }(), |
There was a problem hiding this comment.
We can move that to repository. Data that comes from it should be ready for use in ui without any modifications like that.
There was a problem hiding this comment.
Actually, I am not so sure about this. It would be advisable to use a phone number passed by API. Sometimes it return 2 phone numbers with on in parentheses. For instance: "telephone_number": "
713202721 (+48 665 291 063)
". With moving this logic to repository we wouldn't display the second numberThere was a problem hiding this comment.
However, this output is random. Sometimes this phone number is formatted with spaces and sometimes not. It would be good to generalize it but then we wouldn't return 2 numbers, so idk
There was a problem hiding this comment.
Make the model class has list instead of single field
There was a problem hiding this comment.
Copilot reviewed 10 out of 16 changed files in this pull request and generated no comments.
Files not reviewed (6)
- lib/features/digital_guide_view/amenities/presentation/amenities_expansion_tile_content.dart: Language not supported
- lib/features/digital_guide_view/general_info/presentation/digital_guide_view.dart: Language not supported
- lib/features/digital_guide_view/general_info/presentation/widgets/accessibility_button.dart: Language not supported
- lib/features/digital_guide_view/general_info/presentation/widgets/headlines_section.dart: Language not supported
- lib/features/home_view/widgets/science_clubs_section.dart: Language not supported
- lib/l10n/app_pl.arb: Language not supported
|
what's the status here? |
I'll update it tonight and we are ready to merge |
What has been done
Be careful
I used this icon for the sign language interpreter. I am not sure whether I can... I cannot use one from the digital guide website because none of the buildings is actually using it.
Question
Which accessibility button? I'd personally go for option 3 or 4