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.

Sign up for GitHub

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

Jump to bottom

NTP clock day period component (AM / PM) is huge #2439

Closed
petemill opened this issue Dec 11, 2018 · 11 comments · Fixed by brave/brave-ui#247 or brave/brave-core#1062
Closed

NTP clock day period component (AM / PM) is huge #2439

petemill opened this issue Dec 11, 2018 · 11 comments · Fixed by brave/brave-ui#247 or brave/brave-core#1062
Assignees
@petemill
Labels
QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Test-Plan-Specified QA/Yes regression release-notes/include
Milestone
0.58.x - Release ...

Comments

@petemill
Copy link
Member

Chromium71 changed the type for DayPeriod from 'dayperiod' to 'dayPeriod' which means our matching fails and we end up showing the 'AM' / 'PM' / local-specific-whatever in the same font-size as the time itself.
@petemill petemill added this to the 0.58.x - Beta milestone Dec 11, 2018
@petemill petemill self-assigned this Dec 11, 2018
@petemill petemill mentioned this issue Dec 11, 2018
Merged
@SilverPuppy
Copy link

SilverPuppy commented Dec 11, 2018 •
edited by petemill
Loading

How is this different from brave/brave-ui#247, where the fix is taking place? Was this created by mistake?

@petemill
Copy link
Member Author

petemill commented Dec 11, 2018 •
edited
Loading

@SilverPuppy this is the reported issue, brave/brave-ui#247 is the code to fix in a dependency

@petemill petemill mentioned this issue Dec 11, 2018
Merged
18 tasks
petemill added a commit to brave/brave-core that referenced this issue Dec 11, 2018
@petemill
Update 0.59.x to latest brave-ui 0.59.x
8642d0f 
Fix brave/brave-browser#2439 - Clock AM / PM font size
Use brave-ui welcome images instead of brave-core

```
*   4e07f89 - (59 minutes ago) Merge pull request #315 from brave/revert-sync-59 — Pete Miller (HEAD -> brave-core-0.59.x, origin/brave-core-0.59.x)
|\
| * fb7deb8 - (69 minutes ago) Revert "sync v2" — Cezar Augusto
| * c5e16c7 - (69 minutes ago) Revert "Merge pull request #308 from brave/sync_img" — Cezar Augusto
|/
* c4694e2 - (20 hours ago) Merge pull request #247 from brave/c71-clock-period — Pete Miller
* a2443d8 - (4 days ago) do not count first letter if space in textareaclipboard — Cezar Augusto
* 2c16ae2 - (5 days ago) Merge pull request #308 from brave/sync_img — Cezar Augusto
* 7d9811c - (3 weeks ago) sync v2 — Cezar Augusto
* 3e3be2f - (4 days ago) update snapshot from walletSummary — Cezar Augusto
* a4c4dbd - (5 days ago) Merge pull request #309 from brave/welcome_img — Pete Miller
```
This was referenced Dec 11, 2018
Merged
Merged
@kjozwiak
Copy link
Member

@petemill re: QA, we basically need to verify that AM & PM match the example given in brave/brave-ui#247 correct? They should be smaller and in the right hand corner rather than AM & PM being the same size as the time correct?

@petemill
Copy link
Member Author

@kjozwiak yes, same as previous release

@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Dec 13, 2018 •
edited by LaurenWags
Loading

Verification passed on

Brave 0.59.6 Chromium: 71.0.3578.80 (Official Build) beta (64-bit)
Revision 2ac50e7249fbd55e6f517a28131605c9fb9fe897-refs/branch-heads/3578@{#860}
OS Windows

Verified passed with

Brave 0.58.12 Chromium: 71.0.3578.80 (Official Build) (64-bit)
Revision 2ac50e7249fbd55e6f517a28131605c9fb9fe897-refs/branch-heads/3578@{#860}
OS Mac OS X

screen shot 2018-12-13 at 10 28 21 am

screen shot 2018-12-13 at 1 28 48 pm

@srirambv
Copy link
Contributor

This fails on Linux. Here's the recording https://drive.google.com/open?id=1GMl-4xRTovTsy_SMxix0dTRZ5TLAWuGf Follow up issue logged #2530

@petemill
Copy link
Member Author

It doesn’t seem like that’s a fail for this issue, which is just about the font size when the component is being displayed. The time in that video is using 24hr time format, so we wouldn’t show AM / PM in the previous font size or the corrected font size. I assume that the bug you’ve found - using 24hr time format or 12hr time format - is present on previous version too.

@srirambv
Copy link
Contributor

Whats the expected clock format for NTP? I assumed it should be 12hrs format irrespective of device clock settings/format.

@petemill
Copy link
Member Author

Quite possibly (it’s locale / OS dependent maybe?) but this issue is only about the font size when it does display and fixing that regression. What you’re describing sounds unrelated to this bug.

@srirambv
Copy link
Contributor

Updated the issue title there in #2530. Its definitely not following the system format. @brave/legacy_qa can anyone try on their VM and check if they see 12/24 hr format based on system settings

@LaurenWags
Copy link
Member

@srirambv 12hr format worked for me on Linux VM using
screen shot 2018-12-13 at 4 15 04 pm

screen shot 2018-12-13 at 4 13 03 pm

@kjozwiak kjozwiak mentioned this issue Dec 19, 2018
Closed
@srirambv srirambv mentioned this issue Aug 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@petemill petemill

Labels
QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Test-Plan-Specified QA/Yes regression release-notes/include
Projects
None yet
Milestone
0.58.x - Release (Release Channel)
Development

Successfully merging a pull request may close this issue.

Fix NTP clock for c71 brave/brave-ui
Update to latest brave-ui master brave/brave-core
7 participants
@petemill @kjozwiak @srirambv @SilverPuppy @LaurenWags @btlechowski @GeetaSarvadnya