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

refactor: improve announcements pages #530

Open
wants to merge 32 commits into
base: develop
Choose a base branch
from

Conversation

JoaoCoelho2003
Copy link

@JoaoCoelho2003 JoaoCoelho2003 commented Sep 11, 2024

  • Add working images uploader to the create announcement page.
  • Refactor the existing announcement pages.
  • Improve the announcement feed design.

#435

@ruilopesm ruilopesm changed the title Jc/improve announcements refactor: improve announcements pages Sep 11, 2024
Copy link
Member

@ruilopesm ruilopesm left a comment

Choose a reason for hiding this comment

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

There's a lack of padding on every page. Make sure the padding obeys the one imposed by the page component. In the future, we should make page's body be padded automatically 💡

@FilipeR13 FilipeR13 linked an issue Sep 12, 2024 that may be closed by this pull request
@FilipeR13
Copy link
Contributor

I think it would be cool if the show page followed the same design as the edit/new announcement page when there’s an image to display. What do you think?

@MarioRodrigues10
Copy link
Member

We can have 2 approaches for this scenario:
image

We either limit the characters at index page and if the user wants the full information should open it and see the whole announcement context, or we can keep the whole text and remove the show page since won't be necessary anymore.

Which one do you like more @JoaoCoelho2003 ?

I would also like your input for this case @joaodiaslobo @ruilopesm . 🙌

@JoaoCoelho2003
Copy link
Author

JoaoCoelho2003 commented Sep 13, 2024

I personally prefer limiting the number of lines shown per announcement on the respective pages @MarioRodrigues10.

@joaodiaslobo
Copy link
Member

@MarioRodrigues10

I would also like your input for this case @joaodiaslobo @ruilopesm . 🙌

Keeping the whole text in the feed is definitely not the way to go but I also don't think we should limit what the organizations can write way too much (there still needs to exist a limit, but a bigger one).
Maybe we should truncate the text on the post and allow the users to see the full content in the show page by clicking on the it.

@MarioRodrigues10
Copy link
Member

MarioRodrigues10 commented Sep 13, 2024

@joaodiaslobo There's already a lot of text being truncated at the application in different ways, in order to achieve that I think @JoaoCoelho2003 can create a general function at helpers.ex and truncate for this case only.

In the future, other people should reuse that function and truncate their text aswell if they need.

@JoaoCoelho2003
Copy link
Author

@joaodiaslobo

Keeping the whole text in the feed is definitely not the way to go but I also don't think we should limit what the organizations can write way too much (there still needs to exist a limit, but a bigger one). Maybe we should truncate the text on the post and allow the users to see the full content in the show page by clicking on the it.

In my opinion, we should truncate the text in the feed and, as Mario said, for the user to see the full post he would have to press it and check the announcement. About the text size limit, probably put a limit of around 500 characters or something like that.

image

The text size limit varies a lot depending on the platform, but since this is an announcement, and announcements typically don’t require much text as they are meant to be brief, we could consider setting the limit lower.

@MarioRodrigues10
Copy link
Member

MarioRodrigues10 commented Sep 13, 2024

I think 500 characters could be too much, check Instagram UI/UX for posts in either desktop or mobile I really like what they did, they truncate based on the text size.

@ruilopesm
Copy link
Member

We already have a function for truncating text, if I'm not mistaken. maybe_slice_string/2 (we could change its name to truncate now that I think of it) 🙏


use AtomicWeb, :component

def announcement_card(assigns) do
Copy link
Contributor

Choose a reason for hiding this comment

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

image
Should be a bottom border in the last announcement

Comment on lines +17 to +25
<p class="text-sm font-medium text-zinc-700"><%= @announcement.organization.name %></p>
<p class="text-xs text-gray-500">
<span class="sr-only">Published on</span>
<time><%= relative_datetime(@announcement.inserted_at) %></time>
</p>
</div>
</div>
<div class="px-4 py-2">
<p class="text-lg font-semibold text-zinc-900" title={@announcement.title}>
Copy link
Contributor

Choose a reason for hiding this comment

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

The activitys component is using text-gray instead of text-zinc. This is a issue in all code base, we should normalize this in another PR

</div>
<%= if @announcement.image do %>
<div class="h-auto w-full overflow-hidden">
<img class="h-full w-full rounded-xl object-cover" src={Uploaders.Post.url({@announcement.image, @announcement}, :original)} alt="Announcement Image" />
Copy link
Contributor

Choose a reason for hiding this comment

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

image
I think in the mobile version the image doenst look really good with this level of rounded

</.tab>
</.link>
</.tabs>
<.tabs></.tabs>
Copy link
Contributor

Choose a reason for hiding this comment

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

This solution to add the border in the page its not that good 😅, but I dont really know other solution. @joaodiaslobo any idea?

Copy link
Member

Choose a reason for hiding this comment

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

The page component actually has a parameter to do this, called bottom_border. Unfortunately, it appears this functionality stopped working on a previous PR 😅.
@JoaoCoelho2003 Feel free to fix it and use it on this PR by making the following change in the page component:

def page(assigns) do
    ~H"""
    <div class="flex min-h-full flex-col items-stretch justify-between lg:flex-row">
      <div class="min-h-[100vh] flex w-full flex-col bg-white lg:flex-row lg:border-r">
        <main class="relative z-0 mb-10 flex-1 overflow-y-auto focus:outline-none xl:order-last">
          <div class={["mx-auto max-w-5xl px-4 sm:px-6 lg:px-8", @bottom_border && "border-b"]}>
            <div class="my-6 flex min-w-0 flex-row items-center justify-between"}>
              <h1 class="flex-1 select-none truncate text-2xl font-bold text-gray-900">
                <%= @title %>
              </h1>
              <%= render_slot(@actions) %>
            </div>
          </div>

          <%= render_slot(@inner_block) %>
        </main>
      </div>
    </div>
    """
  end

And use it like this:

<.page title="Announcements" bottom_border>
...

By the way, this PR is looking real good! 🤩
Good job! 💪

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.

Update announcements page (user & admin) frontend
5 participants