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

Feature: Room adverts #804

Merged
merged 30 commits into from
Oct 29, 2024
Merged

Feature: Room adverts #804

merged 30 commits into from
Oct 29, 2024

Conversation

Ellen-Wittingen
Copy link
Contributor

@Ellen-Wittingen Ellen-Wittingen commented Sep 9, 2023

Added room adverts. They are accessible via http://localhost:4200/room-adverts and via the menu-item in the left sidebar.

There is also a view for unauthorized users on the public part of the website, it is accessible via http://localhost:4200/public/room-forum and via the menu-item "Kamerforum" in the "Over ons" dropdown.

Please also check if the English translations are proper English, as nobody except from me has looked at them so far.

Note: it requires the following amber-api branch to function properly: csvalpha/amber-api#393 NOTE BY MATTEO: this backend branch has already been incorporated into staging

Copy link
Contributor

@DrumsnChocolate DrumsnChocolate left a comment

Choose a reason for hiding this comment

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

The implementation works as expected, but I see you haven't quite used ember octane. I know that lots of the entire app is still written in the old style, but please try to follow the checklist (most important): https://guides.emberjs.com/release/upgrading/current-edition/glimmer-components/
and if relevant the rest of the octane upgrade guide mentioned there.
For example, named arguments means we should be using @roomAdvert.location in our templates, not roomAdvert.location
And we should not be using import Component from '@ember/component'; in any of our new components. Always use native classes for everything, and for components only ever extend glimmer components.

translations/en.yaml Outdated Show resolved Hide resolved
translations/en.yaml Outdated Show resolved Hide resolved
app/components/header-nav.js Outdated Show resolved Hide resolved
@DrumsnChocolate
Copy link
Contributor

The text can become rather long, and the text is not vertically bounded in display:
afbeelding

@DrumsnChocolate
Copy link
Contributor

afbeelding
When logged in, I don't see any shorttext preview of the written content of the advert. Is this something we should add?

@DrumsnChocolate
Copy link
Contributor

These are the main UX points I found. I will leave a code review later, this Friday evening probably

@Ellen-Wittingen
Copy link
Contributor Author

Fixed the long scrolling content. Now there is a "lees meer" button that opens a modal with a scrollable bigger view of the advert when clicked upon. That modal is also opened when clicking anywhere on the advert.

image

image

@Ellen-Wittingen
Copy link
Contributor Author

Fixed not showing a short description on room advert index page:

image

@DrumsnChocolate DrumsnChocolate changed the title Added room adverts Feature: Room adverts Oct 14, 2024
@DrumsnChocolate
Copy link
Contributor

it think it would be better if the date could be select through a small calender instead off text. This also should be a required field in my opinion

I am inclined to agree

when on the external website going to the kamerforum page, scrolling down opening a post and closing this post again.
you're unable to scroll

unable to reproduce

Why did we hide the kamerforum below over ons instead of giving it its own place in the header, I thing this would be more appropriate cause people see it soon instead of needing to look in a drop down menu

take a look at the designs supplied by promocie. if you disagree, let's first get this merged and then have a talk to the promocie.

Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 4.65116% with 82 lines in your changes missing coverage. Please review.

Project coverage is 13.25%. Comparing base (7b79a90) to head (f6fff65).

Files with missing lines Patch % Lines
app/components/header-nav.js 5.00% 19 Missing ⚠️
app/controllers/public/room-forum.js 0.00% 7 Missing ⚠️
app/router.js 0.00% 7 Missing ⚠️
app/abilities/room-advert.js 0.00% 6 Missing ⚠️
app/routes/room-adverts/index.js 0.00% 5 Missing ⚠️
app/routes/room-adverts/new.js 0.00% 5 Missing ⚠️
app/controllers/room-adverts/index.js 0.00% 4 Missing ⚠️
app/controllers/room-adverts/new.js 0.00% 4 Missing ⚠️
app/controllers/room-adverts/room-advert/edit.js 0.00% 4 Missing ⚠️
app/routes/room-adverts/room-advert.js 0.00% 4 Missing ⚠️
... and 7 more
Additional details and impacted files
@@             Coverage Diff             @@
##           staging     #804      +/-   ##
===========================================
- Coverage    13.44%   13.25%   -0.19%     
===========================================
  Files          439      455      +16     
  Lines         3110     3175      +65     
===========================================
+ Hits           418      421       +3     
- Misses        2692     2754      +62     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DrumsnChocolate
Copy link
Contributor

I am not sure if we can change this but the house name box gets recognized as address field instead of the address field itself. changing this would make it easier to fill in a advert.

what browser was this again? then I can check if this happens to me too

regarding the input box for available from, I'm working on that: csvalpha/amber-api#439
I will add changes here in ui still.

@lodewiges
Copy link
Contributor

it was in Microsoft edge

@Ellen-Wittingen
Copy link
Contributor Author

So everything is good, except that the "available from" field will be changed to a date instead of text by @DrumsnChocolate ? So do I still have to do anything or will you guys merge it when you are done?

@DrumsnChocolate
Copy link
Contributor

So everything is good, except that the "available from" field will be changed to a date instead of text by @DrumsnChocolate ? So do I still have to do anything or will you guys merge it when you are done?

I'll merge it when I've taken care of the remaining two points: autofill address field, and the type change of the availableFrom field.

@DrumsnChocolate
Copy link
Contributor

DrumsnChocolate commented Oct 28, 2024

@Ellen-Wittingen I do not have any issues with the form no longer submitting when I reapply my change. Are you sure my change broke things?

EDIT: Turns out I can't read

@DrumsnChocolate
Copy link
Contributor

what am I doing lmao, that's a completely different form

@DrumsnChocolate
Copy link
Contributor

depends on csvalpha/amber-api#439

@DrumsnChocolate DrumsnChocolate added this pull request to the merge queue Oct 29, 2024
Merged via the queue into staging with commit c7bb827 Oct 29, 2024
3 checks passed
@DrumsnChocolate DrumsnChocolate deleted the feature/rooms branch October 29, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants