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

[3.0] Theme #7933

Draft
wants to merge 95 commits into
base: release-3.0
Choose a base branch
from
Draft

[3.0] Theme #7933

wants to merge 95 commits into from

Conversation

live627
Copy link
Contributor

@live627 live627 commented Dec 4, 2023

This is the new theme started by @TwitchisMental

I still need to convert several form grids to use CSS grids instead of floats. Also the grids abuse definition list markup, something tat has always annoyed me from the very beginning.

Submitting as a draft now to get early feedback.

@live627 live627 added the Theme label Dec 4, 2023
@Sesquipedalian Sesquipedalian added this to the 3.0 Alpha 4 milestone Dec 4, 2023
<li>
<a href="', Config::$scripturl, '?action=profile"', !empty(Utils::$context['self_profile']) ? ' class="active"' : '', ' id="profile_menu_top">';

if (!empty(User::$me->avatar))
echo User::$me->avatar['image'];

echo '<span class="textmenu">', User::$me->name, '</span></a>
echo '</a>
Copy link
Member

Choose a reason for hiding this comment

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

If we do this for the avatar image, shouldn't we use a alt text for the name?

Copy link
Member

Choose a reason for hiding this comment

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

Should use href or url then.
Or should 'image' provide it by default? It would be good in case an external avatar is missing in other parts of the forum.

Copy link
Member

Choose a reason for hiding this comment

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

I think either is valid. I believe it just needs something to be valid for screen readers. Since its a link to the users profile and we removed the name, the alt should have it.

Copy link
Member

Choose a reason for hiding this comment

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

Then I think 'image' should provide the alt text by default. An external avatar could suddenly not load or disappear, would be good to have it.

@live627
Copy link
Contributor Author

live627 commented Dec 28, 2023

I notice that a few forms would disable textareas on submit, such as the posting form. I've changed this to do all forms by using the new HTML property to make the entire form inert.

@live627 live627 force-pushed the theme branch 2 times, most recently from 3ba4ff5 to c823a49 Compare December 28, 2023 06:58
@dragomano
Copy link
Contributor

Icons in the admin area are not indented enough:
sshot-11

Unable to load the '' template at Forum Maintenance - Routine:
sshot-12

SCEditor doesn't have a dark theme?
sshot-13

Instead of "X Posts" and "Y Topics", maybe it would still be better to keep "Posts: X" and "Topics: Y" as it is now?
sshot-14

@DiegoAndresCortes
Copy link
Member

Icons in the admin area are not indented enough: sshot-11

I’m working on this

SCEditor doesn't have a dark theme? sshot-13

Can be fixed later too, but also requires some tweaks.
Check this topic: https://www.simplemachines.org/community/index.php?topic=586626.0

Instead of "X Posts" and "Y Topics", maybe it would still be better to keep "Posts: X" and "Topics: Y" as it is now? sshot-14

I don’t have a preference, we might discuss it later for sure.

@live627 live627 force-pushed the theme branch 2 times, most recently from 2322f68 to ada494b Compare December 29, 2023 03:14
@jdarwood007 jdarwood007 changed the title Theme [3.0] Theme Jan 20, 2024
@jdarwood007
Copy link
Member

@live627 Can you rebase this or merge and fix conflicts?

@DiegoAndresCortes
Copy link
Member

We have this pending
live627#44

Will start sending more in upcoming weeks, but it could be merged, rest of changes are more focused so can be in separated PR's

@live627
Copy link
Contributor Author

live627 commented Jan 30, 2024

Once I can get around to testing and merging that, I'll rebase this branch again

<div class="windowbg form_grid">
<p class="descbox">', Lang::$txt['authentication_options'], ':</p>
<p>
<input type="radio" name="reminder_type" id="reminder_type_email" value="email" checkeiv></label>
Copy link
Contributor

@dragomano dragomano Feb 9, 2024

Choose a reason for hiding this comment

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

checkeiv => checked?

<input type="submit" value="', Lang::$txt['save'], '" name="save_reserved_names" tabindex="', Utils::$context['tabindex']++, '" class="button">
<div class="form_grid">
<div>
<label for="matchword">', Lang::$txt['admin_match_whole'], '</div>
Copy link
Contributor

@dragomano dragomano Feb 9, 2024

Choose a reason for hiding this comment

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

<label>...</div> => <label>...</label>?

@live627
Copy link
Contributor Author

live627 commented Feb 20, 2024

@jdarwood007 I moved some of the login javascript to its own file but don't know how to test cors requests.

@jdarwood007
Copy link
Member

When this is closer to ready I can check it. You need to run 2 domains (or 2 subdomains) and have the forum on one and the script on another. A simple SSI page where you can open the user area popup is good enough for most things. Sending credentials such as on the login form may also need to be tested, but when you can open the user area, the CORS request works.

@live627
Copy link
Contributor Author

live627 commented Dec 18, 2024

@DiegoAndresCortes any reason for the font size to be so... large?

@DiegoAndresCortes
Copy link
Member

I used 18px as the max because I tested with a 4k monitor.
I'm using this calculator: https://clamp.font-size.app

16px is too small when the resolution is too high imo, but perhaps 17px would be better

@live627
Copy link
Contributor Author

live627 commented Dec 20, 2024

I wonder if that's why half of the random websites I load have huge text. I'm probably not going to be the only one to complain of the huge new font size.

@DiegoAndresCortes
Copy link
Member

I wouldn't know, I'm probably the only freak around here running 4k screens at 100%
I tested 17px and seems fine, we can probably ask Sir Osis to get better feedback on this (https://www.simplemachines.org/community/index.php?topic=589763.0)

@live627 live627 linked an issue Dec 21, 2024 that may be closed by this pull request
@live627
Copy link
Contributor Author

live627 commented Dec 23, 2024

I'm planning on extracting both the board index and the message index into their own seperate template helper functions. Which file shall they go in? I have them in a new file Custom.template.php but they could also go in GenericControls. Thoughts?

@DiegoAndresCortes
Copy link
Member

DiegoAndresCortes commented Dec 23, 2024

Yes please.
Generic is fine...
We could also just do more such as Board.template and move the info center to InfoCenter.template ?

@live627
Copy link
Contributor Author

live627 commented Dec 25, 2024

On second thought, might just keep them in their files of primary use, similar to generic admin/profile options templates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't load more styles to sceditor Quote Selected Text does not appear on Mobile Browser(s)
8 participants