[#8652] Kiez Profile List View #5894
Conversation
sevfurneaux
force-pushed
the
sf-2024-12-kiez-search-profile
branch
from
e2d7d3b to
397de51
Compare
sevfurneaux
changed the title
sevfurneaux
force-pushed
the
sf-2024-12-kiez-search-profile
branch
from
397de51 to
d40b319
Compare
sevfurneaux
force-pushed
the
sf-2024-12-kiez-search-profile
branch
2 times, most recently
from
7aad134 to
de6a04c
Compare
vellip
force-pushed
the
sf-2024-12-kiez-search-profile
branch
from
de6a04c to
278dcee
Compare
| display: inline-block; | ||
| text-align: center; | ||
| cursor: pointer; | ||
| color: var(--color-black); |
There was a problem hiding this comment.
i think we use sass variables here. $text-base if i remember correctly?
There was a problem hiding this comment.
$text-base is now added 👍
| export default function SearchProfiles (props) { | ||
| return ( | ||
| <> | ||
| <h1>{titleText}</h1> | ||
| <p>{descriptionText}</p> | ||
| <SearchProfileList {...props} /> | ||
| </> | ||
| ) | ||
| } |
There was a problem hiding this comment.
split components into separate files please
There was a problem hiding this comment.
All components now split 👍
| return ( | ||
| <div className="search-profile"> | ||
| <div className="search-profile__header"> | ||
| <h3 className="search-profile__title">{profile.name}</h3> |
There was a problem hiding this comment.
It should also show what filters have been set, no?
There was a problem hiding this comment.
Yep, this is now added. I'll add the toggle below the filters in a future PR.
| } | ||
|
|
||
| .search-profile__button:hover, | ||
| .search-profile__button:focus { |
There was a problem hiding this comment.
I would skip the focus style as that normally already has the focus-highlight ring. We don't underline links on focus anywhere else on the page, normally.
There was a problem hiding this comment.
.search-profile__button:focus is now removed.
| left: 0; | ||
| right: 0; | ||
| pointer-events: none; | ||
| position: fixed; |
There was a problem hiding this comment.
I would suggest we keep the normal alertlayout, as it's more consistent to the user that way. afaik regular alerts are not fixed but in document flow
There was a problem hiding this comment.
Now using the normal alert classes 👍
| <h2 className="search-profiles-list__title"> | ||
| {yourSavedProfilesText} ({searchProfiles.length}) | ||
| </h2> |
There was a problem hiding this comment.
You can keep both the h2s in each of the cases (length === 0 // length !== 0) below the alert and just change the text inside the h2 based on the case and then skip the React.Fragments (<>)
There was a problem hiding this comment.
<h2> moved and fragment removed 👍
| const response = await fetch(apiUrl + profile.id + '/', { | ||
| headers: { | ||
| 'Content-Type': 'application/json; charset=utf-8', | ||
| 'X-CSRFToken': cookie.get('csrftoken') | ||
| }, | ||
| method: 'PATCH', | ||
| body: JSON.stringify({ name: e.target.elements.name.value }) | ||
| }) |
There was a problem hiding this comment.
you can use updateItem for this i gather.
There was a problem hiding this comment.
Added updateItem 👍
| ) | ||
| } | ||
|
|
||
| function Buttons ({ onEdit, onDelete, loading }) { |
There was a problem hiding this comment.
Could you give this some more descriptive name after you split the components into separate files? Otherwise this would probably show up for a lot of autocompletes in IDE
There was a problem hiding this comment.
Renamed to SearchProfileButtons
| ) | ||
| } | ||
|
|
||
| function Alert ({ onClose }) { |
There was a problem hiding this comment.
Can you re-use node-modules/adhocracy4/static/Alert.jsx? (path might not be 100% exact)
There was a problem hiding this comment.
I attempted to add this:
import Alert from 'adhocracy4/adhocracy4/static/Alert'
and
<Alert
type="success"
message={alertHeadlineText}
onClick={() => setAlert(false)}
/>But this component looks outdated. It doesn't use the the latest alert classes such as alert__content, alert__headline and alert__text.
It is also not possible to pass in both a headline and text prop, and only a singular message.
I've kept my SearchProfileAlert for now, but maybe we could resolve the above in a PR and add a ticket to make Alert up-to-date?
@vellip Happy New Year! Thanks for the feedback, will take a look now 👀 |
Describe your changes
This PR adds the Kiez profile list view.
Tasks