Skip to content

Comments

Private Messages default template remove bg-light class#42124

Closed
MacJoom wants to merge 2 commits intojoomla:5.0-devfrom
MacJoom:bg-light-darkmode-fix
Closed

Private Messages default template remove bg-light class#42124
MacJoom wants to merge 2 commits intojoomla:5.0-devfrom
MacJoom:bg-light-darkmode-fix

Conversation

@MacJoom
Copy link
Contributor

@MacJoom MacJoom commented Oct 12, 2023

Pull Request for Issue #42121

Summary of Changes

bg-light class removed from div classes

Testing Instructions

send a private message - read the private message

Actual result BEFORE applying this Pull Request

light background in dark mode hard to read text

Expected result AFTER applying this Pull Request

normal background

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • [x ] No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • [ x] No documentation changes for manual.joomla.org needed

@Quy
Copy link
Contributor

Quy commented Oct 12, 2023

Changes in light mode as seen by removing it from the first field, but it should be fine to fix in dark mode.

light-private-messages

@brianteeman
Copy link
Contributor

Simply removing the background is not an ideal solution as the colour was used as a visual clue that the field is read only. For the input fields it used the css below

@media (prefers-color-scheme: dark)
.form-control:disabled, .form-control[readonly] {
    background-color: var(--gray-800);
}

.form-control:disabled, .form-control[readonly] {
    background-color: var(--gray-200);
    opacity: 1;
}

@MacJoom
Copy link
Contributor Author

MacJoom commented Oct 12, 2023

Simply removing the background is not an ideal solution as the colour was used as a visual clue that the field is read only. For the input fields it used the css below

@media (prefers-color-scheme: dark)
.form-control:disabled, .form-control[readonly] {
    background-color: var(--gray-800);
}

.form-control:disabled, .form-control[readonly] {
    background-color: var(--gray-200);
    opacity: 1;
}

You are right - i think best would be to have a bg-readonly class

@joomla-cms-bot joomla-cms-bot added the NPM Resource Changed This Pull Request can't be tested by Patchtester label Oct 12, 2023
@Quy
Copy link
Contributor

Quy commented Oct 12, 2023

I have tested this item ✅ successfully on a1aac82

Thank you!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42124.

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Oct 12, 2023

@MacJoom why all this extra CSS when you could fix everything only in the HTML side, ie:

<form action="<?php echo Route::_('index.php?option=com_messages'); ?>" method="post" name="adminForm" id="adminForm">
    <div class="card">
        <div class="card-body">
            <fieldset>
                <div class="form-group">
                    <div class="mb-3">
                        <label for="userName" class="form-label"><?php echo Text::_('COM_MESSAGES_FIELD_USER_ID_FROM_LABEL'); ?></label>
                        <input type="text" disabled class="form-control" id="userName" value="<?php echo $this->item->get('from_user_name'); ?>">
                    </div>
                    <div class="mb-3">
                        <label for="date" class="form-label"><?php echo Text::_('COM_MESSAGES_FIELD_DATE_TIME_LABEL'); ?></label>
                        <input type="text" disabled class="form-control" id="date" value="<?php echo HTMLHelper::_('date', $this->item->date_time, Text::_('DATE_FORMAT_LC2')); ?>">
                    </div>
                </div>
                <div class="form-group">
                    <div class="mb-3">
                        <label for="subject" class="form-label"><?php echo Text::_('COM_MESSAGES_FIELD_SUBJECT_LABEL'); ?></label>
                        <input type="text" disabled class="form-control" id="subject" value="<?php echo $this->item->subject; ?>">
                    </div>
                </div>
                <div class="form-group">
                    <div class="mb-3">
                        <label for="message" class="form-label"><?php echo Text::_('COM_MESSAGES_FIELD_MESSAGE_LABEL'); ?></label>
                        <textarea disabled class="form-control" id="message" rows="20">
                            <?php echo $this->item->message; ?>
                        </textarea>
                    </div>
                </div>
                <input type="hidden" name="task" value="">
                <input type="hidden" name="reply_id" value="<?php echo $this->item->message_id; ?>">
                <?php echo HTMLHelper::_('form.token'); ?>
            </fieldset>
        </div>
    </div>
</form>

0 new classes, 0 new CSS Variables and it works by default with light/dark/themed!
Bonus it's also accessible!

Screenshot 2023-10-12 at 21 30 47

@ceford
Copy link
Contributor

ceford commented Oct 13, 2023

I have a problem. I am testing on 5.0.0-rc3-dev newly updated this morning with git pull and npm ci. I have two Super Users and one Administrator. In Private Messages: Write the Select a User field does not list any users so I am stuck. Can't test.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42124.

@HLeithner HLeithner added this to the Joomla! 5.0.1 milestone Oct 13, 2023
@wilsonge
Copy link
Contributor

Agree with @dgrammatiko here

@Quy Quy added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Oct 13, 2023
@wilsonge
Copy link
Contributor

OK just looked again and actually not that easy (I guess I'm the idiot). This is the read view. We don't want the textarea being parsed as html and showing the tags when showing the user a private message. We want to render the actual HTML.

I think the mistake here is that we pretend these are form fields when they clearly aren't. This is only a form for the hidden input fields and to facilitate the reply button at the top. There's no scenario when these become editable (because the edit.php is obviously for that).

@Quy Quy removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Oct 13, 2023
@wilsonge
Copy link
Contributor

wilsonge commented Oct 13, 2023

Here's a proposal #42135 of a way that removes all the input like elements and uses dt/dd's. If we decide to go back towards the more input/form like methods then do what @dgrammatiko suggested and make them formally invisible. Right now I assume that the label and elements aren't linked the way we intend them from an a11y perspective (although I assume @brianteeman knows better than me - so feel free to contradict).

@toivo
Copy link
Contributor

toivo commented Oct 14, 2023

I have tested this item ✅ successfully on a1aac82

Tested successfully in 5.0.0-rc3-dev of 14 October using PHP 8.2.11 in Wampserver 3.3.1, in Light mode and Dark mode in Chrome.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42124.

@richard67
Copy link
Member

What to do now with this PR? It has 2 successful human tests so I could set it to RTC, but it seems to me that it would become obsolete when PR #42135 would be merged, which already is RTC.

@wilsonge
Copy link
Contributor

If we want to treat this like a form field we should use formal input fields with labels as @dgrammatiko mentioned.

If we’re happy rendering this without form field style rendering we should merge my PR and maintainers or a release lead should decide which approach

@MacJoom MacJoom closed this Oct 16, 2023
@MacJoom
Copy link
Contributor Author

MacJoom commented Oct 16, 2023

Closing in favour of #42135

@Quy Quy removed this from the Joomla! 5.0.1 milestone Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants