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

Frontpage rework #256

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Frontpage rework #256

wants to merge 28 commits into from

Conversation

B-i-t-K
Copy link
Member

@B-i-t-K B-i-t-K commented May 4, 2024

API:
added two new queries.

  • ctfsByDate(year:int, month:int) return all the CTFs taking place during a specific month
  • `ctftimeCtfById(id: number) return the CTF information from CTFTime (manager+ only)

UI change:

  • removed incomming/past CTFs and calendar page, instead use a by month display
  • removed import CTF dialog, instead use the same dialog as edit, the CTFTime input now has a button to refresh the informations.
  • The front page should be responsive, I've tested on my device but more tests are welcome.

Routing:

  • removed: /ctfs/incomming, /ctfs/past, /ctfs/calendar
  • added: /ctfs/:year?/:month?

Added dependencies:

  • V-calendar: quasar calendar is too primitive for what I'm doing now

Do we want to remove ImportCtf mutation and IncommingCtf PastCtf queries in the future?

- removing of past/ctf/calendar tab, instead use a by month index.
- remove the import ctf, it's now merged into the editCtfDialog
@JJ-8 JJ-8 self-requested a review May 4, 2024 18:47
Copy link
Contributor

@XeR XeR left a comment

Choose a reason for hiding this comment

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

I just deployed this today on TFNS's instance.
(This is not "testing in prod", this is "dogfooding").

I only browsed the different pages with a computer screen.
I did not look at the interface from a mobile/tablet.
I did not look at the code.

The players from TFNS have no choice but use this new interface.
I believe more pain points will arise in the next few days :-)

CTF list on the main page

Today is May the 30th ; there is no CTF between today and the end of the month.
The main page is filled with all the past CTF we've done in May.
I think we always want to have the very next CTF at the top of the main page.

Also it means that, I have to scroll past the CTF we've already did earlier this month.
For example, we'll see the Codegate CTF every time we open CTFNote until July the 1st.

Main page

CTFtime logo

I am not sure if this is only for my specific screen resolution, but it looks like the CTFtime logo is slightly off
CTFtime logo

Update: this is done on purpose, as if it were a ribbon that wraps around the CTF card

CTF Archive

The CTF Archive page should show CTF either sorted by start time or end time (asc or desc)
Since this page includes previous and next CTF, I have a preference for start time asc.
If it only included past CTF I would have had a preference for end time desc.

I believe they are ranked by import date (id), so they are a bit all over the place
CTF archive

Ideally, we would also be able to click on any of the column header to have it sorted by this column.

CTF end date

As mentioned by somebody on IRC, the start date and end date of CTF is the same.
startEnd

For the record, this is what the database contains for this specific event

       start_time       |        end_time        
------------------------+------------------------
 2024-06-01 01:00:00+00 | 2024-06-02 01:00:00+00

Main page title

The main page has no title, it keeps the previous title.
For example, after a successful log in, I end up on the main page but the title is CTFNote - Login

wrong title

Invalid calendar -> CTF card link

When two CTF are overlapping, clicking on a specific day in the calendar should point to the first CTF that is active during that day.

In our instance, there are two CTF for Saturday the 15th:
15th

The justCTF 2024 teaser starts the 15th at 08:00Z.
The Midnight Sun CTF 2024 Finals starts at 10:00Z.

They are displayed in the correct order on the list.
However, they are displayed in the wrong order (sorted by id?) on the calendar.
When I click on the 15th, it goes to the Midnight Sun, hiding the justCTF (I have to scroll up)

When I click a day, the first CTF card should always be the oldest CTF that is still active that day.
In this situation, it should jump to the justCTF because it starts 2 hours earlier.

I suspect this is sorted by the internal id from the database :

 id |            title             |       start_time       |        end_time        
----+------------------------------+------------------------+------------------------
 86 | justCTF 2024 teaser          | 2024-06-15 08:00:00+00 | 2024-06-16 08:00:00+00
 85 | Midnight Sun CTF 2024 Finals | 2024-06-15 10:00:00+00 | 2024-06-16 10:00:00+00

Current cards wrongly show the end date as the start date of a CTF.
@JJ-8
Copy link
Collaborator

JJ-8 commented Jul 14, 2024

@XeR, @B-i-t-K, how well was the new interface received by the team? I checked out the new interface too and I think it looks like a big improvement! But the CTF archive button in the dropdown menu is too much hidden and counter intuitive. I think the button should be positioned somewhere below or in the calendar on the front page.

@XeR
Copy link
Contributor

XeR commented Jul 21, 2024

how well was the new interface received by the team?

Pretty good overall.
The major pain point comes from the CTF list that shows the first CTF of the month.

But the CTF archive button in the dropdown menu is too much hidden and counter intuitive. I think the button should be positioned somewhere below or in the calendar on the front page.

I personally don't use it as much. I usually "search" for a CTF/task name
So I have no opinion on this. Maybe @B-i-t-K does.

@B-i-t-K
Copy link
Member Author

B-i-t-K commented Jul 29, 2024

The ui is in my opinion better, but there is still some bug I want to fix before merging.

  • When a ctf is at the end of the month you need to scroll down to find it
  • I've put some scroll anchor for mobile, but on desktop it's awful, I just need apply them according to the window size.

I don't have time right now to do the changes, but I will have some freetime after DEFCON to do more updates

Copy link
Collaborator

@JJ-8 JJ-8 left a comment

Choose a reason for hiding this comment

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

Amazing work! I really like the auto scrolling functionality and the design in general. I only have a few small remarks, see the reviews but overall the code and UI looks very good!

{{
date
.buildDate({ year: group.year, month: group.month })
.toLocaleDateString(undefined, { year: 'numeric', month: 'long' })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you use toLocaleDateString which will translate the current month to the browser defined language. This makes the UI inconsistent: we use English everywhere except here. I would suggest to keep it English here too.

Comment on lines +41 to +47
<q-card-section>
<div class="text-center text-h5 q-pt-md">No CTFs this month</div>
</q-card-section>
<q-card-section class="text-center">
<q-icon name="hotel" size="128px" />
</q-card-section>
</q-card>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this q-card-section is a bit too big. On mobile it will occupy a full screen to display that there is no information available. On desktop it is also quite large. I would suggest to lower the size and padding so more information would fit on one screen.

Comment on lines +53 to +65
<q-item
v-if="me.isLogged"
clickable
:to="{ name: 'ctfs-archive' }"
active-class="dimmed-background"
>
<q-item-section side>
<q-avatar icon="archive" />
</q-item-section>
<q-item-section>
<q-item-label>CTF Archive</q-item-label>
</q-item-section>
</q-item>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still want to suggest to move this to the calendar as an extra button there. It does not make sense to me to hide this button in the dropdown menu for user settings.

Comment on lines 82 to 102
async function handleNavigation(date: ShortDate) {
void router.replace({
name: 'ctfs',
params: { year: date.year, month: date.month },
});
await doFetchMore(date);
void nextTick(() => {
document
.querySelector(`[data-date="${shortDate.toId(date)}"]`)
?.scrollIntoView({
behavior: 'smooth',
block: 'start',
});
});
if (shortDate.lt(date, start.value)) {
start.value = shortDate.copy(date);
}
if (shortDate.gt(date, end.value)) {
end.value = shortDate.copy(date);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have this annoying bug that will not trigger by a refresh but does trigger when navigating back to the front page (video explains more than words):
ctfnote bug scrolling.webm

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